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

horizontal display for addon-knobs radios UI #3922

Merged

Conversation

astrotim
Copy link

Issue:

Recently merged "radios" knob (#3894) is a great new addition from @brycemhammond. The existing implementation, however, displays the radio inputs vertically, and since space can be limited in the addons panel, I propose that the radio inputs be displayed horizontally.

What I did

Added three lines of CSS to display the radios knobs UI horizontally.

Before

before

After

after

How to test

Is this testable with Jest or Chromatic screenshots?
No.
Does this need a new example in the kitchen sink apps?
No.
Does this need an update to the documentation?
No.

If your answer is yes to any of these, please make sure to include it in your PR.

For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@Primigenus
Copy link
Contributor

This wouldn't work for all situations since some people might use the addons panel docked to the side and with very little horizontal space available. Longer labels for each radio button would also run off the side.

@astrotim
Copy link
Author

@Primigenus The CSS includes flexWrap: wrap so the radio inputs will wrap across multiple lines, rather than running off the side. But I get what you're saying about users that have the addons panel docked to the side.

How about I include a display prop with horizontal | vertical options?

@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

Merging #3922 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3922   +/-   ##
=======================================
  Coverage   39.58%   39.58%           
=======================================
  Files         431      431           
  Lines        5456     5456           
  Branches      739      739           
=======================================
  Hits         2160     2160           
  Misses       2914     2914           
  Partials      382      382
Impacted Files Coverage Δ
addons/knobs/src/components/types/Radio.js 82.35% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3360aef...84c0de9. Read the comment docs.

@Primigenus
Copy link
Contributor

LGTM with flexWrap! Thanks!

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

I'd prefer to use a button group for horizontal display instead. Otherwise it will be too confusing if you have one radio group next to another:
bildschirmfoto 2018-07-28 um 15 20 46

@astrotim
Copy link
Author

@Hypnosphi what exactly do you mean by "button group"? Are you referring to styling the inputs as buttons which was popularised by Bootstrap? Are you requesting this to be developed as a different knob to 'radios', or are you requesting that the existing 'radios' knob have an option to style it as a group of buttons?

IMO, I don't think the screenshot you've included looks confusing. The two sets of radios each have an obvious label and are separated by a dividing line.

@Hypnosphi
Copy link
Member

Hypnosphi commented Jul 29, 2018

Yes, something like this (don't look at the styles, it's a random screenshot from google):

I think it should work as "inline" option for radio knob or something like that. Actually, we could merge select and radio knobs together because they’re are just different presentations of the same data type. It could have some more generic name, like "options" or "enum". It would be more consistent with other knob names as they represent the data type not the actual ui control.

If we're going to do that, right now is the best time because we're approaching a major release and can introduce breaking changes

@Hypnosphi
Copy link
Member

I don't think the screenshot you've included looks confusing

Good for you =) As for me, it requires a bit of mental effort comparing to conventional vertical radios

@astrotim
Copy link
Author

I think adding styling like this introduces a bunch of edge case overhead, primarily when the buttons don't fit within the available horizontal space. I don't really want to go down that road.

@Hypnosphi
Copy link
Member

That's why it's good to have it as an option: people could use it or not depending on how they position their addons panel

@astrotim
Copy link
Author

Sure, that's fair enough, but I'd prefer to scope the horizontal layout of native UI to the option, rather than introduce a different UI altogether.

@Primigenus
Copy link
Contributor

How about defaulting to a vertical list if the panel is docked on the side and to a horizontal layout if it's docked at the bottom?

@ndelangen
Copy link
Member

Here's an idea:

The select, radios are actually similar if not equal in implementation but their display is just different.
I'd suggest we merge this as is, and at a later stage we reform/refactor into 1 generic "pic a few from a list of options" knob that has different display-modes.

One of those can be what you're suggesting @Hypnosphi and others can choose what they prefer. That way everyone wins.

@astrotim
Copy link
Author

@ndelangen awesome idea! #winning

On the subject of display modes, I've recently been wondering if the select UI could be updated to use react-select under the hood. I might have a crack at it...

@ndelangen
Copy link
Member

@Hypnosphi do you agree with this action?

@astrotim If you're going to continue contributing I'd to invite you to the storybook organisation. Would you be able to do a online meeting some time this week?

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 2, 2018

@ndelangen yes, this is basically what I've suggested in #3922 (comment) (2nd paragraph)

It would be nice to do that in 4.0

@ndelangen ndelangen merged commit 9e87a74 into storybookjs:master Aug 2, 2018
@ndelangen
Copy link
Member

Yes let'd do that! I have a online meeting with with @astrotim tomorrow early morning 8 UTC+2. You can of course join if you'd like? 👍

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