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.

Sign up for GitHub

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

Jump to bottom

Improve NcSettingsSelectGroup - Convert to NcSelect #4120

Merged
merged 4 commits into from Jun 22, 2023
Merged

Improve NcSettingsSelectGroup - Convert to NcSelect #4120

merged 4 commits into from Jun 22, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented May 15, 2023 •
edited

Adding documentation, move to NcSelect and drop unused property

  • Moved from deprecated NcMultiselect to NcSelect (Ref: Use NcSelect instead of NcMultiselect in the components #3743 )
  • Debounce search API calls (200ms) to prevent spamming the API while typing
  • Cache empty search groups (the ones loaded when the search dropdown is opened) for current page
  • Load display names of initial groups (those which were given through the value property).
  • Removed unused hint property
  • Added placeholder property and added a hidden label element
    The label property is still used for the placeholder for backwards compatibility.
  • Added documentation for the events and added an example (Add axios mocking so we can emulate querying for groups on the netlify docs)

@susnux susnux added 3. to review Waiting for reviews feature: settings Related to the settings component labels May 15, 2023
@raimund-schluessler
Copy link
Contributor

Supersedes #4104, I guess.

@susnux
Copy link
Contributor Author

susnux commented May 15, 2023

😮‍💨 Somehow I missed that PR

But yes, basically this is fixing the same issue (+ more).

@susnux susnux requested a review from nickvergessen May 15, 2023 17:18
@raimund-schluessler raimund-schluessler added this to the 8.0.0 milestone May 15, 2023
@raimund-schluessler
Copy link
Contributor

Is it the desired behaviour that the groups load only after one types into the input field? This way, no groups show up when one just opens the NcSelect with the arrow-down. But I can imagine that loading all groups by default might be to much on large instances!?

@susnux susnux force-pushed the feat/improve-ncsettingsselectgroup branch from 972965d to c5c8a76 Compare May 15, 2023 20:37
@susnux
Copy link
Contributor Author

susnux commented May 15, 2023

@raimund-schluessler it is (and was) limited to 10 groups per search / api call. I have now pushed another commit for caching at least the initial groups, so that if you use the component multiple time only one API call for an empty search is performed.
Not sure if session storage is overkill, but the library does not use any vue store (like Pinia, etc).

@nickvergessen
Copy link
Contributor

All good, I can close mine

@susnux susnux requested a review from ShGKme May 16, 2023 12:12
nickvergessen
nickvergessen reviewed May 16, 2023
View reviewed changes
src/components/NcSettingsSelectGroup/NcSettingsSelectGroup.vue Outdated
Comment on lines 114 to 141
return this.getValueObject()
return this.getValueObject
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is only needed to make it usable in general, right?
Would it make sense to backport it to 7.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I would partially backport this

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do that? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@susnux please backport to version 7, then we need to release it and update the version in server stable27 and stable26

@susnux susnux requested a review from nickvergessen May 16, 2023 20:52
@nickvergessen nickvergessen mentioned this pull request May 17, 2023
Merged
2 tasks
@susnux susnux force-pushed the feat/improve-ncsettingsselectgroup branch 2 times, most recently from 48ae6dd to fc0ce59 Compare May 22, 2023 20:42
@susnux susnux requested a review from nickvergessen May 22, 2023 20:42
@susnux

This comment was marked as outdated.

Sign in to view
nickvergessen
nickvergessen requested changes May 25, 2023
View reviewed changes
Copy link
Contributor

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Selection is still somewhat broken. I can also reproduce on styleguide

  1. Manually change the groups: [] and add groups: ['foo']

grafik

  1. Open the dropdown
  2. No more options are shown
  3. Clear the selection using the "X" on foo
  4. Force reload
  5. Selection is not usable anymore

src/components/NcSettingsSelectGroup/NcSettingsSelectGroup.vue Outdated Show resolved Hide resolved
@susnux
feat(NcSettingsSelectGroup): Adding documentation, move to NcSelect a…
e86085f 
…nd drop unused property

* Moved from deprecated `NcMultiselect` to `NcSelect`
* Removed unused `hint` property
* Added `placeholder` property and added a hidden label element
  The `label` property is still used for the placeholder for backwards compatibility.
* Added documentation for the events and added an example

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
feat(docs): Add axios mocking for the documentation to allow examples…
dacbabc 
… of the `NcSettingsSelectGroup`

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
fear(NcSettingsSelectGroup): Reduce API calls for loading groups
46c7eef 
* Debounce search requests when typing
* Cache initial groups (empty search) for all components of current page

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the feat/improve-ncsettingsselectgroup branch from fc0ce59 to 46c7eef Compare May 31, 2023 15:06
@susnux
feat(NcSettingsSelectGroup): Filter by display name *and* id of group
227134b 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented May 31, 2023

@nickvergessen I think it is solved finally (there was a reactivity issue with vue).

@susnux susnux requested a review from nickvergessen May 31, 2023 16:19
nickvergessen
nickvergessen approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Seems to work now, yes

@nickvergessen nickvergessen merged commit 5a5f2b2 into master Jun 22, 2023
16 checks passed
@nickvergessen nickvergessen deleted the feat/improve-ncsettingsselectgroup branch June 22, 2023 07:59
This was referenced Jun 22, 2023
Closed
Closed
@skjnldsv skjnldsv mentioned this pull request Jun 28, 2023
Merged
@nickvergessen nickvergessen mentioned this pull request Jul 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@raimund-schluessler raimund-schluessler raimund-schluessler approved these changes

@ShGKme ShGKme Awaiting requested review from ShGKme

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
3. to review Waiting for reviews feature: settings Related to the settings component
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@susnux @raimund-schluessler @nickvergessen