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

Feat pro 1274 pinkv2 combobox #225

Open
wants to merge 12 commits into
base: v2
Choose a base branch
from
Open

Feat pro 1274 pinkv2 combobox #225

wants to merge 12 commits into from

Conversation

ernstmul
Copy link
Contributor

Adds Combobox:
image

Have you read the Contributing Guidelines on issues?

✅

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Oct 11, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 289c2b6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

💥 An error occurred when fetching the changed packages and changesets in this PR 
Some errors occurred when validating the changesets config:
The package or glob expression "@appwrite.io/pink" specified in the `linked` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@appwrite.io/pink-icons" specified in the `linked` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@appwrite.io/kitchensink" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@appwrite.io/fonts" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@appwrite.io/pink-design" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@vercel Vercel
Copy link

vercel bot commented Oct 11, 2024 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
pink-design ❌ Failed (Inspect) Oct 15, 2024 1:10pm
ui-kitchensink ❌ Failed (Inspect) Oct 15, 2024 1:10pm

@github-actions GitHub Actions
Copy link
Contributor

Uh oh! @ernstmul, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

ArmanNik
ArmanNik requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@ArmanNik ArmanNik left a comment

Choose a reason for hiding this comment

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

The list doesn't scroll when using arrows to navigate
https://github.com/user-attachments/assets/46219e60-5862-4d51-a77b-17e3e7ee2ff3

Clicking on option doesn't select it, work only by pressing enter
https://github.com/user-attachments/assets/5ee12b56-1162-4c2b-bab1-f37ff3be4a55

v2/pink-sb/src/lib/combobox/Combobox.svelte Outdated
Comment on lines 85 to 97
<svg
width="20"
height="20"
viewBox="0 0 20 20"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<path
fill-rule="evenodd"
clip-rule="evenodd"
d="M5.29289 7.29289C5.68342 6.90237 6.31658 6.90237 6.70711 7.29289L10 10.5858L13.2929 7.29289C13.6834 6.90237 14.3166 6.90237 14.7071 7.29289C15.0976 7.68342 15.0976 8.31658 14.7071 8.70711L10.7071 12.7071C10.3166 13.0976 9.68342 13.0976 9.29289 12.7071L5.29289 8.70711C4.90237 8.31658 4.90237 7.68342 5.29289 7.29289Z"
/>
</svg>
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the Icon component here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep we can! Didn't use that one before! Good suggestion, thanks!

v2/pink-sb/src/lib/combobox/Combobox.svelte
export let disabled: ComboboxProps['disabled'] = false;
export let options: ComboboxProps['options'] = [];
export let label: ComboboxProps['label'] = '';

Copy link
Member

Choose a reason for hiding this comment

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

We might need the required state like for other inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added $$restProps to the input

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the $$restProps is enough... I think we might need to show a * on the label too when it's required (double check with Caio)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the $$restProps came in handy because we extend the props from HTMLInputAttributes, so we get all those optional props right there. Asked Caio about the *

@ernstmul
Copy link
Contributor Author

#225 (review)
These are addressed

ArmanNik
ArmanNik reviewed Oct 17, 2024
View reviewed changes
Copy link
Member

@ArmanNik ArmanNik left a comment

Choose a reason for hiding this comment

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

The scroll behavior is still weird... when I keep the arrow pressed the selected element goes beyond the fold and then the scroll "catches up" once I stop pressing the arrow

Also, should we group this with the other inputs? We might want to use the same wrapper (Base) on this component too for consistency... wdyt?

v2/pink-sb/src/lib/combobox/Combobox.svelte
export let disabled: ComboboxProps['disabled'] = false;
export let options: ComboboxProps['options'] = [];
export let label: ComboboxProps['label'] = '';

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the $$restProps is enough... I think we might need to show a * on the label too when it's required (double check with Caio)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArmanNik ArmanNik ArmanNik requested 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
@ernstmul @ArmanNik