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

[Dropdown] Height variation #587

Merged
merged 3 commits into from
Mar 26, 2019
Merged

Conversation

exoego
Copy link
Contributor

@exoego exoego commented Mar 25, 2019

Description

This PR adds height variation to dropdown module so that more items can be glanced at once.
This is helpful if dropdown have tons of items (e.g. Countries, States/Providences, Divisions, something like that).

This is usable if every items can be shown at once within the height of window without scroll bar.
If there are so many items that requires scroll bar, user can consider columnar variation proposed in #586.

Screenshot (when possible)

image

Closes

Closes #545

lubber-de
lubber-de previously approved these changes Mar 25, 2019
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM

@lubber-de lubber-de added type/feat Any feature requests or improvements lang/css Anything involving CSS state/awaiting-reviews Pull requests which are waiting for reviews labels Mar 25, 2019
@lubber-de lubber-de added this to the 2.7.4 milestone Mar 25, 2019
Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

Just a semantic note here: hasn't taller more sense that higher ?

@lubber-de
Copy link
Member

Higher/ highest or taller/tallest... I am fine with both 😀 what does our native speaker @hammy2899 say?

@lubber-de lubber-de added the state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo label Mar 25, 2019
@y0hami
Copy link
Member

y0hami commented Mar 26, 2019

IMHO taller doesn't make much sense without more context. The word tall is an adjective so is used to describe something. In this case tall dropdown means that the dropdown is taller then the default height because its describing the height of the dropdown so the user assumes the height will be taller then what it is originally. When using taller dropdown it doesn't have any context of what it is taller than. A good example to this is you don't say "That person is taller" you would say "That person is tall" but with some context you could say "That person is taller than that building" which would make sense.

I think adding words for height will have just as much of a problem as the size modifiers. In FUI we have mini, tiny, small, medium, large, big, huge, massive (in order from smallest to largest) and even English speaking users have problems with deciding which is largest or smaller than the other. Take big and large for example, personally I would argue large should be larger then big but it isn't. This is even more of a problem when none English speaking users try to distinguish them.

I think we need to think more about the terminology we use before we add anything to the framework so we can prevent breaking changes in the future.

@prudho
Copy link
Contributor

prudho commented Mar 26, 2019

And what about long and very long, which are already used by the Placeholder component ?

@y0hami
Copy link
Member

y0hami commented Mar 26, 2019

@prudho That's a good point and that would also be reusing current terminology.

ref: https://fomantic-ui.com/elements/placeholder.html#line-length

Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

@exoego could you please replace higher and highest classes by long and very long to reuse our current terminology. Thanks !

@exoego
Copy link
Contributor Author

exoego commented Mar 26, 2019

Changed to long and very long in 5921029

prudho
prudho previously approved these changes Mar 26, 2019
Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami
Copy link
Member

y0hami commented Mar 26, 2019

We should also add short and very short so it stays consistent.

@exoego
Copy link
Contributor Author

exoego commented Mar 26, 2019

I don't think shorter dropdown make sense in terms of end-user readability (showing less items).
But it may be useful if combining with columnar like short columnar dropdown.

If you think consitency is more important, what factors should be for very short and short ?
Is it ok for 0.5 for very short and 0.75 for short?

  very short short standard long very log
factor 0.5x 0.75x 1x 2x 3x
"@selectionMobileMaxItems" 1.5 2.25 3 6 9
"@selectionTabletMaxItems" 2 3 4 8 12
"@selectionComputerMaxItems" 3 4.5 6 12 18
"@selectionWidescreenMaxItems" 4 6 8 16 24

(unit: items)

@lubber-de
Copy link
Member

We should also add short and very short so it stays consistent.

🤔 How should it behave then? So which max-height setting should be set when using short or very short.... i would leave the PR as it is now

lubber-de
lubber-de previously approved these changes Mar 26, 2019
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami
Copy link
Member

y0hami commented Mar 26, 2019

I think the table @exoego shows is perfect, I would agree that short and very short will not be used much but its best to keep the consistency especially when its a simple change.

@exoego exoego dismissed stale reviews from lubber-de and prudho via c3b4eca March 26, 2019 10:47
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami modified the milestones: 2.7.4, 2.7.3 Mar 26, 2019
@y0hami y0hami merged commit 32dec40 into fomantic:develop Mar 26, 2019
@y0hami y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label Mar 26, 2019
@exoego exoego deleted the taller-dropdown branch March 29, 2019 10:05
y0hami pushed a commit to fomantic/Fomantic-UI-Docs that referenced this pull request Apr 1, 2019
@y0hami y0hami mentioned this pull request Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants