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(SupportedDevicesTable): Group list by brands #251

Merged
merged 29 commits into from
Jun 29, 2023

Conversation

mikewuu
Copy link
Contributor

@mikewuu mikewuu commented Jun 28, 2023

closes #234

Notable things

  • Demo will show the 'View...all devices' button if the list has > 1 device, but it's actually set to 3.
  • If a brand has <= 3 devices, it will just show all the models.
  • Wasn't sure about the spacing when a brand is collapsed, opted to remove the 32px spacing. No longer relevant, see feat(SupportedDevicesTable): Group list by brands #251 (comment).

Screenshots

CleanShot 2023-06-29 at 04 03 31

CleanShot.2023-06-29.at.04.06.28.mp4

src/lib/brands.ts Outdated Show resolved Hide resolved
@dawnho
Copy link
Contributor

dawnho commented Jun 28, 2023

Is there a way to add some additional devices to the seed data, so that the story can capture the overlay behavior?

@dawnho
Copy link
Contributor

dawnho commented Jun 28, 2023

The section expansion / compression logic is a bit wrong.

The section is supposed to compress back down to the 3-item view.

See the prototype here: https://www.figma.com/proto/6nCfNVHmYQ7wxhnFOpFBm6/Supported-devices?type=design&node-id=171-37116&t=qUp24hA279bNHfHY-0&scaling=min-zoom&page-id=167%3A15042&starting-point-node-id=171%3A37116

@razor-x
Copy link
Collaborator

razor-x commented Jun 28, 2023

Is there a way to add some additional devices to the seed data, so that the story can capture the overlay behavior?

Example data can be updated here https://github.com/seamapi/fake-seam-connect/blob/main/src/pages/api/internal/device_models/list.ts

@razor-x
Copy link
Collaborator

razor-x commented Jun 28, 2023

And the fake will need the /devices/list_device_providers endpoint.

Copy link
Collaborator

@razor-x razor-x left a comment

Choose a reason for hiding this comment

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

Still working though it, but leaving some comments for now. We need to address the fake updates and api usage first anyway.

@mikewuu
Copy link
Contributor Author

mikewuu commented Jun 29, 2023

The section expansion / compression logic is a bit wrong.

The section is supposed to compress back down to the 3-item view.

my bad! I thought it was weird that there was no spec for the spacing.

Updated in 031af09

CleanShot.2023-06-29.at.11.46.06.mp4

@mikewuu mikewuu requested a review from razor-x June 29, 2023 03:51
@mikewuu
Copy link
Contributor Author

mikewuu commented Jun 29, 2023

ok! This should be ready for re-review:

CleanShot.2023-06-29.at.12.50.18.mp4

Copy link
Collaborator

@razor-x razor-x left a comment

Choose a reason for hiding this comment

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

We do need to update the fake seam connect version so we can review the storybook and then we should get a review from @dawnho before merge. So consider this blocked until then.

Found a few quick code suggestions. The naming stuff is non-blocking as it's all internal, but always fun to chat about. Otherwise nothing else major blocking from me.

@dawnho
Copy link
Contributor

dawnho commented Jun 29, 2023

Can we add the brands prop that will allow a user to filter for a subset of the brands?
i.e. <SupportedDeviceTable brands={ ["yale", "schlage"] } /> will return a table that only shows those two brands

@dawnho
Copy link
Contributor

dawnho commented Jun 29, 2023

The accordion looks 🔥 ! Thanks for the fixes!

@mikewuu
Copy link
Contributor Author

mikewuu commented Jun 29, 2023

We do need to update the fake seam connect version so we can review the storybook and then we should get a review from @dawnho before merge. So consider this blocked until then.

Not sure where this needs to be updated for it to work in the Preview. It's already updated in package.json


import { useDeviceProviders } from 'lib/seam/devices/use-device-providers.js'

export function useDeviceProvider(brand: string): DeviceProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to suggest we return null or isLoading while the hook is loading, but maybe the behavior that the provider will show as unknown until it loads or in case it fails is better UX since things will still partially render if this is loading or fails.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mikewuu Will merge this now but leave this comment open to discuses if we want to change this or not.

Copy link
Contributor

@dawnho dawnho left a comment

Choose a reason for hiding this comment

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

Looks great! What does the filter button do, btw? I know nino added that in the designs, but he hadn't got around to creating a filter menu yet. We can hide that if it's not an active button.

@razor-x
Copy link
Collaborator

razor-x commented Jun 29, 2023

@dawnho You should be able to play with the Filter button in the storybook. It looks like this
image

@razor-x razor-x changed the title feat(SupportedDevicesTable): group list by brands feat(SupportedDevicesTable): Group list by brands Jun 29, 2023
@razor-x razor-x merged commit ba241c2 into main Jun 29, 2023
@razor-x razor-x deleted the supported-devices-table branch June 29, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supported Devices Table - Brand sections
4 participants