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 option to display config enums as radio buttons #1089

Merged
merged 11 commits into from Apr 5, 2019

Conversation

Projects
None yet
4 participants
@marnen
Copy link
Contributor

commented Oct 25, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This pull request adds support for a radio option in the config schema, for enum config values. If radio is true, then the enum will be displayed as a group of radio buttons.

Edit by @rsese to add screenshot from #1089 (comment)

image

Alternate Designs

I chose to add an option here because it seemed like the easiest way to not break existing config files, and to maintain backwards compatibility for new config files. If a config file using the radio: true feature is processed by an older version of Atom, the extra key will simply be ignored and the option will be displayed as a <select> element.

Benefits

For small numbers of options, radio buttons often provide a better user experience than a drop-down menu would. This is especially true if the option descriptions are on the long side.

Edit by @rsese to add extra context from #1089 (comment)

If there are fewer than approximately 5 options, radio buttons show them all at a glance, without needing to click. Likewise, it takes only 1 click to select a radio button, as opposed to 2 clicks for a drop-down option. There are other reasons too, but those are the most prominent.

Possible Drawbacks

I designed this to be as free of negative impact as I could manage.

@@ -322,6 +326,13 @@ export default class SettingsPanel extends CollapsibleSectionPanel {
} else {
return floatValue
}
} else if (type === 'integer') {

This comment has been minimized.

Copy link
@marnen

marnen Oct 25, 2018

Author Contributor

I might be able to unify this code with current float-parsing logic.

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

Looks like the only failing test on AppVeyor is one that's been failing intermittently anyway, and so I believe it's unrelated to this pull request.

@rsese

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Thanks @marnen!

Can you share a screenshot of what this look like with your pull request?

Also:

For small numbers of options, radio buttons often provide a better user experience than a drop-down menu would. This is especially true if the option descriptions are on the long side.

Can you share an example of this that shows why radio buttons would be better for a particular option?

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

@rsese Sure, I’ll share a screenshot later when I’m back at my computer.

Can you share an example of this that shows why radio buttons would be better for a particular option?

If there are fewer than approximately 5 options, radio buttons show them all at a glance, without needing to click. Likewise, it takes only 1 click to select a radio button, as opposed to 2 clicks for a drop-down option. There are other reasons too, but those are the most prominent.

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

See also https://blog.prototypr.io/7-rules-of-using-radio-buttons-vs-drop-down-menus-fddf50d312d1 (which I just found this minute). It reinforces what I was saying, but more clearly.

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

Here's the promised screenshot.
image

@rsese

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Thanks! I added the screenshot and the extra explanation of the benefits to the issue body and someone from the team will take a look as soon as they can.

@lee-dohm

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

Would you mind fixing the merge conflict so we can review this?

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Yikes! I wasn’t aware that there was one; sorry about that. Apparently the mobile interface for GitHub doesn’t show that piece of info. I’ll be happy to.

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@lee-dohm Should be resolved now.

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

I see some linter failures. I’ll fix those.

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Really? The linter wants a space between the function name and the parentheses? That seems very odd to me...

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

This should be ready to review and merge now.

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

Just checking: is there anything further preventing this from being reviewed?

@rsese

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

@marnen - sorry it's just a matter of workload, but when one of our engineers can review you'll see comments/questions/etc.

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Sure, understood. I just wanted to make sure that there was nothing else procedural that I had to take care of.

@nathansobo nathansobo self-assigned this Apr 5, 2019

@nathansobo nathansobo merged commit 1d922e7 into atom:master Apr 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I'm assuming you adding explicit parsing for the integer type because before we were allowing it to pass through as a string, which breaks logic associated with choosing the correct radio button? Just going to merge based on that assumption. Seems good to parse the integer regardless.

Thanks for the contribution and sorry for the delay.

nathansobo added a commit to atom/atom that referenced this pull request Apr 5, 2019

nathansobo added a commit that referenced this pull request Apr 5, 2019

Revert "Merge pull request #1089 from marnen/radio-buttons"
This reverts commit 1d922e7, reversing
changes made to 9ea9ad9.

nathansobo added a commit that referenced this pull request Apr 6, 2019

@marnen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

@nathansobo After 5 months, I’m afraid I don’t remember exactly why I put the parsing in, but I do believe that it was as you say, because choosing the correct radio button wasn’t working.

Thanks for merging this! I believe this will help make the settings panel UI a bit more pleasant.

@marnen marnen referenced this pull request Apr 6, 2019

Open

Use radio buttons in config #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.