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

Variant selection components #1034

Closed
wants to merge 5 commits into from
Closed

Variant selection components #1034

wants to merge 5 commits into from

Conversation

chrisFrazier77
Copy link
Contributor

No description provided.

@chrisFrazier77 chrisFrazier77 linked an issue Apr 25, 2024 that may be closed by this pull request
Variant Selection Components #978
Closed
@chrisFrazier77 chrisFrazier77 linked an issue Apr 25, 2024 that may be closed by this pull request
Variant Configuration (mappings) #1035
Closed
korgon
korgon requested changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@korgon korgon left a comment

Choose a reason for hiding this comment

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

Initial review, this is looking good, a few comments and ideas to ponder...

packages/snap-store-mobx/src/types.ts Outdated Show resolved Hide resolved
packages/snap-store-mobx/src/Search/Stores/SearchResultStore.ts Show resolved Hide resolved
packages/snap-store-mobx/src/Search/Stores/SearchResultStore.ts Outdated Show resolved Hide resolved
packages/snap-store-mobx/src/Search/Stores/SearchResultStore.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a TODO on line 364

use sorting function from config

This is something that will likely come up at some point, but just wanted to mention it here that maybe we could add it to the VariantConfig when we merge this with the preSelected config. I can make a task for this.

packages/snap-store-mobx/src/Search/Stores/SearchResultStore.ts Outdated Show resolved Hide resolved
packages/snap-preact-components/src/components/Molecules/Swatches/Swatches.stories.tsx Outdated Show resolved Hide resolved
packages/snap-preact-components/src/components/Molecules/VariantSelection/VariantSelection.tsx Outdated Show resolved Hide resolved
packages/snap-preact-components/src/components/Molecules/VariantSelection/VariantSelection.tsx Outdated Show resolved Hide resolved
packages/snap-preact-components/src/components/Molecules/VariantSelection/VariantSelection.tsx Outdated Show resolved Hide resolved
packages/snap-preact-components/src/components/Molecules/VariantSelection/VariantSelection.tsx Outdated Show resolved Hide resolved
@korgon
Copy link
Contributor

korgon commented Apr 29, 2024

We should keep in mind that images using background will not lazy load; we may want to consider some logic that creates images instead in some cases. Additionally adding a mapping property for image (or similar) would help differentiate that.

@korgon
Copy link
Contributor

korgon commented May 3, 2024 •
edited

Grid component prop ideas:

disableOverflowAction
overflowButton
overflowButtonInGrid
onOverflowButtonClick

Swatch component prop ideas:

grid (enabled + other grid props)
carousel (enabled + other carousel props)

Would be nice to put selection option fieldName as themeName on component.

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

@korgon korgon korgon 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.

Variant Configuration (mappings) Variant Selection Components
2 participants
@chrisFrazier77 @korgon