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

Multiselect fixes #139

Merged
merged 1 commit into from
Dec 3, 2018
Merged

Multiselect fixes #139

merged 1 commit into from
Dec 3, 2018

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Dec 1, 2018

  • Border radius fixes
  • Padding increase
  • Removed triangle on single select
Before After
capture d ecran_2018-12-03_18-16-36 capture d ecran_2018-12-03_18-17-35
capture d ecran_2018-12-03_18-19-44 capture d ecran_2018-12-03_18-19-16

@jancborchardt I did not removed the user icon as this is actually a standard (you can use any icon you want like we do in the sharing with nextcloud/server#11899)

@skjnldsv skjnldsv added enhancement New feature or request 2. developing Work in progress papercut Annoying recurring issue with possibly simple fix. feature: select Related to the NcSelect* components labels Dec 1, 2018
@skjnldsv skjnldsv added this to the 0.4.7 milestone Dec 1, 2018
@skjnldsv skjnldsv self-assigned this Dec 1, 2018
@skjnldsv skjnldsv added 3. to review Waiting for reviews design Design, UX, interface and interaction design and removed 2. developing Work in progress labels Dec 1, 2018
Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good

Would you mind splitting these separate features for future PRs and provide before/after screenshots? That would be great!

@ChristophWurst
Copy link
Contributor

Would you mind splitting these separate features for future PRs and provide before/after screenshots? That would be great!

Background: "Multiselect fixes" is just too generic. There will be many more of these fixes and knowing that has changed in a release of this lib will be hard to understand for anybody using this in their app.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Dec 3, 2018

Background: "Multiselect fixes" is just too generic. There will be many more of these fixes and knowing that has changed in a release of this lib will be hard to understand for anybody using this in their app.

Commit comment is more detailed for this purpose ;)
Let me add some screenshots :)

@ChristophWurst
Copy link
Contributor

Commit comment is more detailed for this purpose ;)

Yep, saw that, thanks! IMO still is a valid concern to have this separated as it also makes it possible to revert a single fix if it causes any issue.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Dec 3, 2018

@ChristophWurst updated op :)

@ChristophWurst ChristophWurst merged commit 4ee2eb4 into master Dec 3, 2018
@ChristophWurst ChristophWurst deleted the multiselect-fixes branch December 3, 2018 19:42
@jancborchardt
Copy link
Contributor

@skjnldsv good stuff, thanks!

did not removed the user icon as this is actually a standard

If I want to propose to only have an icon for when it's not a single user (which is the default) should I just open an issue here?

(Cause right now with all entries having an icon, the icon becomes quite meaningless as it's almost always the same.)

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Dec 4, 2018

I'd say this should be a common sense usage rule then, because it make sense to have the icon for other things like an entry that represent a circle, a talk convo... (or god knows why 🤔 ).
If I get you right, you mean that since the user is the default data item, we should not display the user icon but all the other ones are fine, correct? 😉

@ChristophWurst @juliushaertl I don't think we should prevent this specific icon (icon-user), but on the other hand, if we could be sure devs would follow this common sense usage rule, then we'd be in a dream world! Thoughts? 😁

@jancborchardt
Copy link
Contributor

jancborchardt commented Dec 4, 2018

If I get you right, you mean that since the user is the default data item, we should not display the user icon but all the other ones are fine, correct? 😉

Exactly. :)

I don't think we should prevent this specific icon (icon-user), but on the other hand, if we could be sure devs would follow this common sense usage rule, then we'd be in a dream world!

I also don’t think we should prevent it. But a good start would be:

  • We don’t use it in our example apps or building blocks or in the docs cause that’s how people see what we expect. Which is why I mentioned it here.
  • In the docs we say something like "On the right you can show an icon signifying if it’s a group, a Talk conversation or something else special. If it is just a person, no icon is needed."

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UX, interface and interaction design enhancement New feature or request feature: select Related to the NcSelect* components papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants