Skip to content

Consolidate cursor resets in Reboot#25598

Closed
mdo wants to merge 6 commits intov4-devfrom
reboot-cursor
Closed

Consolidate cursor resets in Reboot#25598
mdo wants to merge 6 commits intov4-devfrom
reboot-cursor

Conversation

@mdo
Copy link
Copy Markdown
Member

@mdo mdo commented Feb 11, 2018

After one of the comments in #25351, I think we can consolidate our cursor resets on non-disabled anchors and buttons into Reboot.

  • Removes pagination link, .navbar-toggler, .close, and .btn cursor: pointer resets.
  • Adds new ones to Reboot for links and buttons.

/cc @XhmikosR @patrickhlauke

@patrickhlauke
Copy link
Copy Markdown
Member

wondering if this doesn't get us back where we started with regards to authors using random elements, for whatever reason, with, say .btn class. like (assuming they did it at least accessibly) <span tabindex="0" role="button" class="btn">Foo</span> or something.

@mdo
Copy link
Copy Markdown
Member Author

mdo commented Feb 20, 2018

With something like that, there's still no reason to not use a <button> element, correct? Point being, we shouldn't encourage that kind of markup when semantic elements work best. Also probably not worth trying to try custom cursors to every class—just adds extra CSS.

@XhmikosR
Copy link
Copy Markdown
Member

I too think it sort of brings us back to the previous situation but it's simpler for us this way.

Now, guys, it's your call, but personally I'd go with this and with the docs stating to prefer the proper element.

@mdo
Copy link
Copy Markdown
Member Author

mdo commented Feb 20, 2018

I'd go with this and with the docs stating to prefer the proper element.

Yeah, noted. I can get some docs updates in here.

Comment thread scss/_reboot.scss

// Reset the `cursor` on non-disabled links. Also applies to buttons.
&:not(:disabled):not(.disabled) {
cursor: pointer;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this would also force cursor:pointer for any anchors (<a name="blah">, or more generally <a> elements without an href). this will likely make react people happy (as they're usually the ones complaining, if memory serves), but will end up with confusing experience on sites that do use static anchors that are not links?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

solution if we did want to just do this for real links would be to change selector to something like &[href]:not(disabled):not(.disabled)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this code necessary? When <a> is a link, browser makes the cursor to pointer by default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah, good point. yes, it seems at that point, it's redundant (and nicely circumvents the whole "what about anchors" dilemma)

@patrickhlauke
Copy link
Copy Markdown
Member

i'll admit i'm conflicted. on the one hand, sure, using semantically correct elements should be the preferred approach. on the other hand we know for a fact not all our authors do it, and then come here and complain that we're not adding the cursor:pointer as part of the class.

but i guess if we actually document this, advise authors to manually add their own cursor:pointer if for whatever reason they can't use the correct element, and strictly point people to that part of the docs when they come here and open issues...that'd probably be ok?

- Removes pagination link, .navbar-toggler, .close, and .btn cursor: pointer resets
- Adds new ones to Reboot for links and buttons
@XhmikosR
Copy link
Copy Markdown
Member

Should we drop this in favor of #27021?

@XhmikosR XhmikosR requested a review from a team as a code owner November 4, 2018 14:13
@MartijnCuppens MartijnCuppens deleted the reboot-cursor branch January 4, 2019 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants