Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix backend data-action across multiple files #2184

Merged
merged 6 commits into from
Aug 29, 2017

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Aug 23, 2017

In backend there are multiple links with inconsistent data-action attributes.

For example data-action="red" or data-action="green", which are both not semantic and do not change tooltip colors when needed (tooltip colors are defined with actions names, not color).

With this PR we also start using these semantic data-action into js to target buttons instead of rely on class names that can change in the future, like icons ones (fa-something).

The only real effect of this PR on the UI is that buttons and tooltips use the right color now:

Stock Management

Before

stock_cancel_before

After

stock_cancel_after

Stock Transfer

Before

stock_transfer_cancel_before

stock_transfer_save_before

After

stock_transfer_cancel_after

stock_transfer_save_after

Store Credits

Before

store_credit_cancel_before

After

store_credit_cancel_after

There's no need to put unsemantic actions like green or red there.
It is better to clearly indicate what that action does, even if it
does not reflect any tooltip color change.
We are uncorrectly using fa-void icons on cancel action.
This will make cancel icons and tooltips take warning color
instead of a wrong error color, when hovering.
js components will not rely anymore on icons implementations.
It will use a semantically improved data-actions of links.
It will survive if we change icons framework, also it is more
samantically correct.
@kennyadsl kennyadsl force-pushed the normalize-data-actions branch from 9c026fe to 4a4cc92 Compare August 23, 2017 21:10
@mtomov
Copy link
Contributor

mtomov commented Aug 24, 2017

Cool that!

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good to me too, thanks a bunch.

Copy link
Contributor

@jhawthorn jhawthorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@kennyadsl kennyadsl self-assigned this Aug 25, 2017
@kennyadsl kennyadsl merged commit bba46c4 into solidusio:master Aug 29, 2017
@kennyadsl kennyadsl deleted the normalize-data-actions branch August 29, 2017 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants