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

feat: Add NcChip component #5686

Merged
merged 5 commits into from
Jun 11, 2024
Merged

feat: Add NcChip component #5686

merged 5 commits into from
Jun 11, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jun 10, 2024

☑️ Resolves

A "modern" alternative to NcUserBubble.
The intended use case is for searches or filters to show as the currently active filters.
The current default size is 24px to align with UI changes (cc @marcoambrosini ).

It allows to set an icon (no legacy classes but icon slot or prop), the text and optionally actions (by default only the close button).

🖼️ Screenshots

Screenshot 2024-06-10 at 17-02-51 Nextcloud Vue Style Guide

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews labels Jun 10, 2024
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Everything looks great

…fault clickable area

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented Jun 11, 2024

(Fixed stylelint issue)

Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Very nice <3

I would just up the trailing padding when there's text only. 6px instead of 4 should work. Possibly we'll have to reduce the icon size a bit too but let's see it in action first :)

@susnux
Copy link
Contributor Author

susnux commented Jun 11, 2024

I would just up the trailing padding when there's text only. 6px instead of 4 should work.

Makes sense, I adjusted the styles for this!

image

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…nextcloud/router` in styleguide

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux merged commit 8ac42de into master Jun 11, 2024
18 checks passed
@susnux susnux deleted the feat/nc-chip branch June 11, 2024 19:56
@susnux
Copy link
Contributor Author

susnux commented Jun 11, 2024

/backport to next

@skjnldsv
Copy link
Contributor

skjnldsv commented Jun 12, 2024

Just so you know the interraction in that component doesn't pass accessibility requirements.
Min clickable area is 44px cc @jancborchardt @nimishavijay

@ShGKme
Copy link
Contributor

ShGKme commented Jun 12, 2024

For the reference: https://www.w3.org/WAI/WCAG21/Understanding/target-size.html

The size of the target for pointer inputs is at least 44 by 44 CSS pixels except when:

Equivalent
The target is available through an equivalent link or control on the same page that is at least 44 by 44 CSS pixels;
Inline
The target is in a sentence or block of text;
User Agent Control
The size of the target is determined by the user agent and is not modified by the author;
Essential
A particular presentation of the target is essential to the information being conveyed.

So, the question is how it is supposed to be used. As I understand, it is kind of an alternative to NcUserBubble, so it is supposed to be used in text?

@susnux
Copy link
Contributor Author

susnux commented Jun 12, 2024

No, it does satisfy the WCAG rules, as we only focus on AA not AAA, so the minimum target size is 24px x 24px.
ref: https://www.w3.org/WAI/WCAG22/Understanding/target-size-minimum.html
Or more specific we want to fulfill BITV which relys on AA not AAA here, ref: https://bitvtest.de/pruefschritt/wcag-22-web/wcag-22-web-2-5-8-zielgroesse-minimum

So the current design roadmap is to shrink our defaul clickable area (probably to 34px x 34px) and only use big buttons (there will be large variant of buttons) that use 44px x 44px area.

it is kind of an alternative to NcUserBubble, so it is supposed to be used in text?

No it is ment for e.g. the unified search to display the current search filter.

@skjnldsv
Copy link
Contributor

I'm more dubious about the menu within that new bubble. That would be very hidden and I doubt this will pass WCAG :)

The inline close icon is probably already discutable 🤔

@susnux
Copy link
Contributor Author

susnux commented Jun 12, 2024

I'm more dubious about the menu within that new bubble. That would be very hidden and I doubt this will pass WCAG :)

There is only a trigger button that has a size of 24px which is the minimum allowed for accessibility, so this will pass WCAG.

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Jun 12, 2024

It will pass, but I agree that it's definitely not a happy place for action menus. I would limit the usage to a close button only @susnux

Do you have any other use cases in mind for advanced usage?

@susnux susnux mentioned this pull request Jun 20, 2024
1 task
@skjnldsv
Copy link
Contributor

skjnldsv commented Jul 3, 2024

@susnux is the ncBubble deprecated then?
When shall we use that one over the older one?

@susnux
Copy link
Contributor Author

susnux commented Jul 3, 2024

@skjnldsv

Currently it is a general purpose component, as the NcUserBubble has a very specific use case any every extension (even the "button" on the right side in the example) is quite hacky.
I would say NcChip is the general purpose component while NcUserBubble should only be used for what it is expected: Showing users / groups.

I had not planned to deprecate the bubble, but would be possible if this is desired.

PS: If you like to use the chip inline you need to add display: inline-flex; vertial-alignment: middle to it.

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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants