Skip to content
New issue

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

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

Already on GitHub? # to your account

D8 core 724 button hover #537

Merged
merged 3 commits into from
Dec 4, 2019
Merged

D8 core 724 button hover #537

merged 3 commits into from
Dec 4, 2019

Conversation

cjwest
Copy link
Member

@cjwest cjwest commented Nov 14, 2019

READY FOR REVIEW

Summary

Hover and focus should rely on something other that a color change. I put in an underline to indicate the change
https://stanfordits.atlassian.net/browse/D8CORE-724

Needed By (Date)

  • When does this need to be merged by?
    Soon.

Urgency

  • How critical is this PR?
    Would like it for DecanterPalooza

Steps to Test

Check out this branch
Verify the button hover, active, and focus work as expected

Affected Projects or Products

Associated Issues and/or People

See Also

@sherakama sherakama temporarily deployed to Tugboat November 14, 2019 23:30 Destroyed
@sherakama
Copy link
Member

@sherakama
Copy link
Member

sherakama commented Nov 15, 2019

Thanks @cjwest,

If you're looking to get rid of the button jumping have a look at the examples on this page for how they did it: https://www.w3.org/TR/wai-aria-practices/examples/button/button.html

I also noticed that their hover state differed from their focus state.

@sherakama sherakama temporarily deployed to Tugboat November 17, 2019 00:19 Destroyed
@yvonnetangsu
Copy link
Member

Hey @cjwest , agree that we need something other than color change for button hover/focus state. What do you think about just adding an underline to the text instead? That would work better in the buttons in the CTA component for example.

@cjwest
Copy link
Member Author

cjwest commented Nov 22, 2019

Hi @yvonnetangsu,
I appreciate your thoughts on this! I think that this should have styling that works well with the rest of the site. I'm happy to switch to underlines.

Let me know if you want to run with it and create a PR. Otherwise, I can propose something.

@sherakama
Copy link
Member

@cjwest go ahead and run with it.

@sherakama sherakama temporarily deployed to Tugboat November 24, 2019 00:20 Destroyed
@cjwest
Copy link
Member Author

cjwest commented Nov 28, 2019

Hi @yvonnetangsu,

I switched from the outline to the underline. Take a look and if you like it, hit "Squash and merge".

@sherakama sherakama temporarily deployed to Tugboat November 30, 2019 20:41 Destroyed
@sherakama sherakama temporarily deployed to Tugboat December 1, 2019 00:26 Destroyed
@yvonnetangsu
Copy link
Member

@cjwest @sherakama Hm....I don't think this PR is for merging into master - looks more like a v6 merge to me 🤔

@cjwest
Copy link
Member Author

cjwest commented Dec 2, 2019

Oh, thank you, @yvonnetangsu, for the clarification! I was debating that.

@yvonnetangsu
Copy link
Member

Oh, thank you, @yvonnetangsu, for the clarification! I was debating that.

Did you do a npm run build, @cjwest ? Looks like all the dist/ asset files were removed.

@sherakama sherakama changed the base branch from master to v6 December 4, 2019 20:00
@sherakama sherakama temporarily deployed to Tugboat December 4, 2019 20:04 Destroyed
@sherakama
Copy link
Member

Proper branch set and compiled.

Ready for review.

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Looks good. GTG!

@sherakama sherakama merged commit 682a3e2 into v6 Dec 4, 2019
@sherakama sherakama deleted the D8CORE-724-button-hover branch December 4, 2019 20:56
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants