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

Add a clear, obvious way to remove hotkeys to improve UX #2883

Merged

Conversation

zbrcz
Copy link
Contributor

@zbrcz zbrcz commented Nov 28, 2017

The solution is based on bradrisse's comment here:
mui/material-ui#2932

Based on that thread, it seems to me that there is currently no better way to add the requested button.

@welcome
Copy link

welcome bot commented Nov 28, 2017

💖 Thanks for opening this pull request! 💖
Here is a list of things that will help get it across the finish line: - Run npm run lint locally to catch formatting errors earlier. - Include tests when adding/changing behavior. - Include screenshots and animated GIFs whenever possible. - If you added new translation strings ensure that you have added empty strings for all languages - If you added new functionality or modified existing functionality please ensure it is tested

@zbrcz zbrcz force-pushed the feature/hotkey-remove-btn branch 2 times, most recently from c23b001 to e831ec1 Compare November 28, 2017 13:17
Copy link
Owner

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Can you please provide a screenshot showing what this looks like 👍

position: 'relative',
},
clearButton: {
'background-color': 'Transparent',
Copy link
Owner

Choose a reason for hiding this comment

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

This can just be backgroundColor and the value should be all lower case 👍

@zbrcz
Copy link
Contributor Author

zbrcz commented Nov 28, 2017

Please note that I have amended the commit. I did it using "git push -f", not sure if that's the right way. :)

The requested screenshot:
hotkeyremovebutton

@jostrander
Copy link
Collaborator

Looks reasonable. Does it also work with the dark theme?

@zbrcz
Copy link
Contributor Author

zbrcz commented Dec 12, 2017

No, the icon doesn't change color. Thanks for pointing that out, I'll take a look at it.

@zbrcz
Copy link
Contributor Author

zbrcz commented Dec 16, 2017

I have changed the <button> to a <FlatButton> and the arrow now changes color according to the theme. How do I submit the change? I get the feeling that git push -f is not the right way.

@jostrander
Copy link
Collaborator

git push -f is the right way if you are squashing your commits every time, otherwise a regular git push would do.

@jostrander
Copy link
Collaborator

@MarshallOfSound this is good to go, just pulled and reviewed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 71.629% when pulling b723456 on zbrcz:feature/hotkey-remove-btn into 125c233 on MarshallOfSound:master.

@MarshallOfSound MarshallOfSound merged commit d0bf8e0 into MarshallOfSound:master Jan 2, 2018
@welcome
Copy link

welcome bot commented Jan 2, 2018

Congrats on merging your first pull request! Have a ⭐

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.

None yet

4 participants