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 chip element #2942

Merged
merged 9 commits into from
May 2, 2024
Merged

Add chip element #2942

merged 9 commits into from
May 2, 2024

Conversation

chrschorn
Copy link
Contributor

@chrschorn chrschorn commented Apr 23, 2024

This adds the Quasar Chip element. Quick demo from the documentation page:

chrome_V0pI9w4ceQ.mp4

I wanted to implement all three main features (clickable, selectable, removable) and my goal was to enable a label add/remove interface as shown in the lower part of the video.

As a part of that, I also added Enter key support for ui.input which can be quite handy. Doing this via ui.keyboard seemed not easily possible as the callback in there does not know which element is focussed at the moment. I realized the way to do this is .on("keydown.enter")

@falkoschindler falkoschindler self-requested a review April 24, 2024 05:35
@falkoschindler falkoschindler added the enhancement New feature or request label Apr 24, 2024
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, @chrschorn! This turned out to be a rather powerful UI element. 🙂

I just reviewed and slightly updated your code:

  • Like for ui.item, I removed the slightly redundand clickable parameter. The chip is automatically clickable when a click handler is provided.
  • I removed the awaitable clicked method. Even though we added it to ui.button to support something like "UI flows", we don't think every clickable UI element needs to support it.
  • You wrote "The update:selected event has no value, so we treat it as a toggle event". But as far as I can tell we can use e.args as the new selection state.
  • For the pytests I replaced some hard assertions with screen.should_contain statements with implicit waits. This can reduce flakiness on less powerful build systems.
  • I improved the contrast of some demo chips and simplified the second demo a bit. Even though I sacrificed the notifications with the complete chip list, I hope the reduced complexity helps to focus on the chip API.
  • Since most of the remaining code in chip.py was basically about managing the selection, I decided to introduce a new mixin. This helps to focus on this single feature and to use other mixins as blueprint for the state synchronization. And this way it was easy to add binding support.

What do you think? Do you agree with the changes I made?

@falkoschindler falkoschindler added this to the 1.4.24 milestone Apr 29, 2024
@chrschorn
Copy link
Contributor Author

Thanks for the review/polish/cleanup! They all make sense, I have nothing to add 👍

@falkoschindler falkoschindler merged commit 54c644f into zauberzeug:main May 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants