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(UiKit): Users select #31455

Merged
merged 38 commits into from
May 24, 2024
Merged

feat(UiKit): Users select #31455

merged 38 commits into from
May 24, 2024

Conversation

tiagoevanp
Copy link
Contributor

@tiagoevanp tiagoevanp commented Jan 15, 2024

This PR introduces new input components to UiKit:

  • Multiple Users Select
  • Single User Select

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

CORE-59

Copy link

changeset-bot bot commented Jan 15, 2024

🦋 Changeset detected

Latest commit: 719635a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 33 packages
Name Type
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/ui-kit Minor
@rocket.chat/meteor Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/core-services Patch
@rocket.chat/core-typings Patch
@rocket.chat/livechat Patch
@rocket.chat/rest-typings Patch
rocketchat-services Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/presence Patch
@rocket.chat/apps Patch
@rocket.chat/cron Patch
@rocket.chat/gazzodown Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/ddp-client Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.23%. Comparing base (526cbf1) to head (719635a).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #31455      +/-   ##
===========================================
- Coverage    56.24%   56.23%   -0.01%     
===========================================
  Files         2428     2428              
  Lines        53560    53560              
  Branches     11031    11031              
===========================================
- Hits         30124    30122       -2     
- Misses       20798    20832      +34     
+ Partials      2638     2606      -32     
Flag Coverage Δ
unit 72.30% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@tiagoevanp tiagoevanp changed the title feat(UiKit): Introduce users/channels selectors feat(UiKit): Introduce users select Feb 14, 2024
@tiagoevanp tiagoevanp marked this pull request as ready for review February 14, 2024 16:52
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Feb 14, 2024
@tiagoevanp tiagoevanp requested a review from a team February 14, 2024 16:53
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Feb 14, 2024
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Feb 14, 2024
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Feb 15, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Feb 15, 2024
Copy link
Member

@gabriellsh gabriellsh left a comment

Choose a reason for hiding this comment

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

I actually had several other changes to request, but there's a bigger issue that I noticed upon further. Some (if not all) of the components created for the fuselage-ui-kit package, are copies of other existing components that are already in use. Even some comments have been copied over, as well as API calls (some have minor changes).

This feels a little strange to me, as we should not be copying implementation like this. Even more so because there are some implementation flaws on these components (like irrelevant useMemos and some weird logic) that was not modified/removed/improved in any sort of form. Could you explain the reasoning behind this? Is there any way we could improve this by reusing the same components?

Also, are we expecting that federated users will be shown in apps? I think this should be discussed since federated users have some limitations on how they work compared to regular users.

Btw, I know this is already a little too critic, but the name chosen for the components Multi....Multiple, doesn't it seem a little... redundant?

@tiagoevanp
Copy link
Contributor Author

In fact, I was copying core implementations, thanks to raise the points where my eyes passed by without notice some of the dumb errors... I will correct them ASAP. Another important thing to mention is: I know that we don't have a perfect autocomplete/select component to use, and we have a lot of different implementations of it in so many parts of the code. I already have created a different task to create a separated package for users/channel selectors who should centralize all... like ui-selects, or something else! I received a negative from another teammate because there is more than a simple work on doing this, regarding fuselage work for select components as well 😬. So, my first option to make some progress was the copycat technique, and wait for future improvements on this side.

gabriellsh
gabriellsh previously approved these changes Apr 16, 2024
@scuciatto scuciatto removed this from the 6.8 milestone Apr 18, 2024
@tiagoevanp tiagoevanp added this to the 6.9 milestone Apr 23, 2024
@jessicaschelly jessicaschelly added the stat: QA assured Means it has been tested and approved by a company insider label Apr 25, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Apr 25, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Apr 26, 2024
@scuciatto scuciatto removed this from the 6.9 milestone May 14, 2024
@tiagoevanp tiagoevanp added the stat: ready to merge PR tested and approved waiting for merge label May 24, 2024
@kodiakhq kodiakhq bot merged commit a565999 into develop May 24, 2024
31 of 32 checks passed
@kodiakhq kodiakhq bot deleted the feat/user-channel-uikit branch May 24, 2024 18:14
gabriellsh added a commit that referenced this pull request May 28, 2024
…retention

* 'develop' of github.com:RocketChat/Rocket.Chat: (36 commits)
  refactor: IntegrationHistory out of DB Watcher (#32502)
  fix: Message update being broadcasted without updated values (#32472)
  test: make api teams test fully independent (#31756)
  test: Fix test name (#32490)
  fix: streams being called with no logged user (#32489)
  feat: Un-encrypted messages not allowed in E2EE rooms (#32040)
  feat(UiKit): Users select (#31455)
  fix: Re-login same browser tab issues (#32479)
  chore: move all webclient code out of the COSS folders (#32273)
  chore(deps): bump thehanimo/pr-title-checker from 1.3.7 to 1.4.1 (#30619)
  fix: Don't show join default channels option for edit user form  (#31750)
  fix: CAS user merge not working (#32444)
  fix: Overriding Retention Policy not working (#32454)
  fix: `rooms.export` endpoint generates an empty export when given an invalid date (#32364)
  fix: "Allow Password Change for OAuth Users" setting is not honored in the "Forgot Password" flow (#32398)
  fix: Bypass trash when removing OTR system messages and read receipts (#32269)
  fix: Monitors dissapearing from Unit upon edit (#32393)
  fix: Link image preview not opening in gallery (#32391)
  feat: Allow visitors & integrations to access downloaded files after a room has closed (#32439)
  regression: Users tab misaligned (#32451)
  ...
matheusbsilva137 pushed a commit that referenced this pull request May 31, 2024
@AliNunes AliNunes modified the milestones: 6.10, 6.9 May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants