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

Replace SelectList with SelectField. #127

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Replace SelectList with SelectField. #127

merged 1 commit into from
Dec 19, 2023

Conversation

willnationsdev
Copy link
Contributor

  • Updates QuickSearch to use SelectField.
  • Removes SelectList.
  • Adds features to SelectField that allow it to support SelectList's use cases.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 12, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 287f6d4

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

This PR includes changesets to release 1 package
Name Type
svelte-ux Minor

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

@vercel Vercel
Copy link

vercel bot commented Nov 12, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-ux ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 14, 2023 5:46am

@what-the-diff What The Diff
Copy link

what-the-diff bot commented Nov 12, 2023

PR Summary

  • Introduction of a new changeset file

    • A new changeset file .changeset/quick-avocados-hope.md has been added. It includes changes that refer to the removal of the SelectList component. Moreover, this file also documents the modifications in the svelte-ux component, particularly the enhancement of SelectField to support the now-removed SelectList use case. Lastly, the QuickSearch component has been updated to use the enhanced SelectField.

  • Updates to QuickSearch.svelte

    • QuickSearch.svelte has been redesigned with the SelectField component used instead of the previous SelectList. The updated QuickSearch.svelte also includes additional feature enhancements like the autoFocus and selectOnFocus actions. The overall look of the component has also been refreshed with new theme and styles.

  • Modifications to SelectField.svelte

    • SelectField.svelte has seen an elimination of the selectOnFocus import. Instead, two new imports have been introduced- autoFocus and selectOnFocus. Additional functionalities are also incorporated through the Maybe and IconInput imports. The overall use case has been updated through the introduction of new properties and the modification of existing ones. Stylistic changes have concluded the upgrades to SelectField.svelte.

  • Removal of SelectList.svelte

    • The SelectList.svelte file has been completely removed as part of these changes.

  • Amendments to index.ts

    • In the index.ts file, the export functionality of SelectList has been eliminated, aligning with the removal of the SelectList.svelte file.

techniq
techniq requested changes Nov 12, 2023
View reviewed changes
Copy link
Owner

@techniq techniq left a comment

Choose a reason for hiding this comment

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

A few comments. I also noticed an opening issue while testing it in the vercel preview I'll share is discrod

packages/svelte-ux/src/lib/components/QuickSearch.svelte Outdated Show resolved Hide resolved
packages/svelte-ux/src/lib/components/SelectField.svelte Outdated Show resolved Hide resolved
packages/svelte-ux/src/lib/components/SelectField.svelte Outdated Show resolved Hide resolved
packages/svelte-ux/src/lib/components/SelectField.svelte Outdated Show resolved Hide resolved
@techniq techniq mentioned this pull request Nov 13, 2023
Merged
2 tasks
@techniq
Copy link
Owner

techniq commented Nov 14, 2023

@willnationsdev I'm not sure if you would want to tackle this as part of the same PR, but I created #132 (extracted from #130) which helps standardize SelectField with MenuField (mostly small tasks)

@willnationsdev willnationsdev mentioned this pull request Dec 11, 2023
Closed
@willnationsdev
Copy link
Contributor Author

Was in the process of implementing changes to incorporate #132, but ran into #152 which is now a blocker that prevents me from fully testing & verifying that whatever changes I've made are actually working properly.

@willnationsdev
Copy link
Contributor Author

@techniq This should now be ready for review. I've included the requested changes from #132.

@techniq
Copy link
Owner

techniq commented Dec 13, 2023

Awesome, thanks @willnationsdev. I don't have a ton of time to look at it right now, but will more this evening.

I did a quick run through using the Vercel preview and noticed QuickSearch has an issue.

CleanShot.2023-12-13.at.14.57.53.mp4

My guess is we just need to update where we build the options to use label instead of name.

@willnationsdev
Copy link
Contributor Author

@techniq QuickSearch should be fixed now. I updated all the SelectField docs, but never updated the label/name difference in the +layout.svelte file, as you suspected.

techniq
techniq requested changes Dec 14, 2023
View reviewed changes
Copy link
Owner

@techniq techniq left a comment

Choose a reason for hiding this comment

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

Looks really good, and I love all the Typescript improvements. Some 2 small nits (and I can handle them post merge if you want.

Could you add the text-left class to <button> within SelectField and make sure it doesn't cause any unexpected regressions, and maybe add a SelectField example with inlineOptions enabled?

packages/svelte-ux/src/lib/components/SelectField.svelte Outdated Show resolved Hide resolved
@willnationsdev
Replace SelectList with SelectField w/ MenuField API sync.
287f6d4 
- Updates SelectField to support the same features as SelectList.
- Updates QuickSearch to use SelectField.
- Declared MenuOption & moved to types folder.
- Updated MenuField/SelectField to reference MenuOption.
- Switched to use of 'label' instead of 'name' for options.
- Added `activeOptionIcon` to SelectField.
@willnationsdev
Copy link
Contributor Author

@techniq Afaik, these changes should be ready for merging. Please review it when you get a chance & let me know if I need to update anything else.

techniq
techniq approved these changes Dec 19, 2023
View reviewed changes
Copy link
Owner

@techniq techniq left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks! We chatted about fixing the chevron button toggle, but we'll get to that at some point :). I'm glad to get this merged so I can restack on top of it for the theme work, and this PR didn't have Cloudflare previews (assuming because it existed before I switched) and looking to shutdown my Vercel integration (save money and stop messaging me about failures because I switched to use the cloudflare sveltekit adapter explicitly).

Thanks again, and sorry it took so long to merge. I'll try to update one of my large projects "soon" and see if I find any unexpected regressions.

@techniq techniq merged commit a2cface into techniq:main Dec 19, 2023
4 checks passed
@github-actions github-actions bot mentioned this pull request Dec 19, 2023
Merged
This was referenced Dec 19, 2023
Closed
Closed
jycouet pushed a commit that referenced this pull request Dec 20, 2023
@willnationsdev @jycouet
Replace SelectList with SelectField w/ MenuField API sync. (#127)
701650c 
- Updates SelectField to support the same features as SelectList.
- Updates QuickSearch to use SelectField.
- Declared MenuOption & moved to types folder.
- Updated MenuField/SelectField to reference MenuOption.
- Switched to use of 'label' instead of 'name' for options.
- Added `activeOptionIcon` to SelectField.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@techniq techniq techniq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@willnationsdev @techniq