-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Display a pointer cursor hovering add variant buttons #2062
Display a pointer cursor hovering add variant buttons #2062
Conversation
Well, I'd say that the problem is in the CSS rather than the markup. Button can even be thought to be better semantically, because the action that you are about to perform is not a Also, I'd doubt that this is the only place, which uses buttons, so applying such a change site-wide using css should a 1 liner. |
@mtomov thanks for your feedback! I initially thought the same thing and I was surprised that buttons do not have |
I don't think these elements should have the "pointer" cursor. I think the buttons which do have the pointer cursor are a mistake due to them being "Buttons shouldn’t have a hand cursor" by Adam Silver argues for this and has some reference to authorities saying the same: Microsoft, Apple, w3c. cc @Mandily who may have some thoughts on the matter |
I'm of the thought that anything interactive on the page should have the affordance of a pointer cursor, or a text cursor if it's an input. I understand where the author of that linked post is coming from, that in theory interactive things should look like they can be interacted with, and therefore don't need the cursor to tell users. But in practice, we have flat designs with less affordance, and the pointer cursor only helps to reinforce. |
I read that article before doing this PR but I don't fully agree with the author. Like a lot of comments do, I also think that it's more important to focus on user expectations. I think that Microsoft and Apple resources linked refer to Desktop application, which maybe is a bit different from a web application. w3c takes a strong position in favor of not using pointer cursors for elements that are not links. Anyway I'm starting to feel that this discussion is too much general and would require a separate issue for discussion. If we decide to go with removing pointer from buttons (and other elements that are not links) we probably have to change a lot of stuff in both backend and frontend markup/style. |
eb7d03a
to
b884a26
Compare
Updated the PR so that we now have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m fine with that change. There are some typos though
@@ -62,9 +62,9 @@ | |||
</td> | |||
<td class="actions"> | |||
<% if can? :update, shipment %> | |||
<%= link_to '', '#', class: 'save-method fa fa-check no-text with-tip', | |||
<%= button_tag '', class: 'save-method fa fa-check no-text with-ti, type: :buttonp', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo at the end of the line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
data: {'shipment-number' => shipment.number, action: 'save'}, title: Spree.t('actions.save') %> | ||
<%= link_to '', '#', class: 'cancel-method fa fa-cancel no-text with-tip', | ||
<%= button_tag '', class: 'cancel-method fa fa-cancel no-text with-ti, type: :buttonp', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both fixed now!
They are not real links so semantically it is better to use button tags instead of anchors.
b884a26
to
ac81e0a
Compare
Big fan of changing empty anchors to buttons 👍 |
thanks! |
The button tag does not display a pointer cursor. Using an anchor tag also uniforms the markup with other similar buttons present in the line items management area.
Before
After