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

SearchGuide: New Component + VR Implementation #3772

Merged
merged 24 commits into from
Oct 4, 2024

Conversation

eniomoura
Copy link
Contributor

@eniomoura eniomoura commented Sep 23, 2024

Summary

What changed?

Implemented the SearchGuide component, and added the VR implementation experiment on top.

Classic:
image
image
image
image

VR:
image
image
image
image

Why?

To bring the web implementation in line with the design spec for Gestalt.

Links

Checklist

  • [ Yes ] Added unit tests
  • [ Yes ] Added documentation + accessibility tests
  • [ Yes ] Verified accessibility: keyboard & screen reader interaction
  • [ Yes ] Checked dark mode, responsiveness, and right-to-left support
  • [ Yes ] Checked stakeholder feedback (e.g. Gestalt designers, relevant feature teams)

@eniomoura eniomoura marked this pull request as ready for review September 23, 2024 08:42
@eniomoura eniomoura requested a review from a team as a code owner September 23, 2024 08:42
@eniomoura eniomoura added the minor release Minor release label Sep 23, 2024
@eniomoura eniomoura changed the title Searchguide v1 component SearchGuide: Component Implementation Sep 23, 2024
@eniomoura eniomoura changed the title SearchGuide: Component Implementation SearchGuide: New Component Sep 23, 2024
Copy link

netlify bot commented Sep 23, 2024

Deploy Preview for gestalt ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 246992a
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/67000e734de777000807d23d
😎 Deploy Preview https://deploy-preview-3772--gestalt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@eniomoura
Copy link
Contributor Author

I added the missing link implementation, but the docs might need a second look. I'll edit it a bit after I'm done with the VR implementation.

@eniomoura eniomoura changed the title SearchGuide: New Component SearchGuide: New Component + VR Implementation Sep 27, 2024
@AlbertCarreras
Copy link
Contributor

AlbertCarreras commented Oct 3, 2024

Screenshot by Dropbox Capture

probably just change the example

For Icon we could force the color with a span, forcing the color. or clonen the object and only allowing

Screenshot by Dropbox Capture

Actually

@AlbertCarreras
Copy link
Contributor

Screenshot by Dropbox Capture

@AlbertCarreras
Copy link
Contributor

AlbertCarreras commented Oct 3, 2024

Avatar seems to have the outline.
Screenshot by Dropbox Capture

Change the exmple...

looking at Toast, we follow the same approach... if we decide to enforce things... we should do it in both components

</Box>
);
return (
<button
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the Toast approach?
Screenshot by Dropbox Capture
Screenshot by Dropbox Capture

Copy link
Contributor Author

@eniomoura eniomoura Oct 4, 2024

Choose a reason for hiding this comment

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

This is what I've been doing, but inline instead of separate components, what/why should change here? Or is this just reinforcing the suggestion above?

<Flex alignItems="center" gap={{ row: 2, column: 0 }} justifyContent="center">
{'avatar' in thumbnail && (
<Box aria-hidden marginStart={isInVRExperiment ? 2 : undefined} minWidth={40}>
{cloneElement(thumbnail.avatar, { size: 'fit' })}
Copy link
Contributor

@AlbertCarreras AlbertCarreras Oct 3, 2024

Choose a reason for hiding this comment

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

could we reuse this?

Not sure if it might work... but it might come handy! more consistency

import {
  AvatarThumbnail,
  IconThumbnail,
  ImageThumbnail,
  Message,
  TypeThumbnail,
} from './sharedSubcomponents/thumbnailSubcomponents';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would require some changes in thumbnailSubcomponents, there's some hardcoded props on it that would break SearchGuide design.
This might be a good follow up PR.

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'll also leave the avatar border for this further look into Toast.

@@ -0,0 +1,76 @@
import { Avatar, AvatarGroup, Flex, Icon, Image, SearchGuideLink } from 'gestalt';
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit, can we avoid the Example word at the end of files? they are in the example folder hahaah


return (
<Flex alignItems="center" gap={2} height="100%" justifyContent="center" width="100%">
<SearchGuide
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: can we have state for each individual one?

Copy link
Contributor

Choose a reason for hiding this comment

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

cn we also add the thumbnails here, to see the selected state?

<Box height={120} overflow="scrollX" padding={4}>
<Flex direction="row" gap={2} wrap>
<SearchGuide color="02" text="Casual" />
<SearchGuide color="02" text="Formal" />
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. use just one where you implement the prop, not 3
  2. change the colors in children
  3. try layer and check if it stays accesible. it should add the focus to the first SearchGuide on keyboard navigation.

That would be a good example. if it breaks let me knwo

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, apply all the accessibilty props!

Reuse this example in the accessibility section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do 1 full implementation in one example, and then another example where we can see all this permutations?
Screenshot by Dropbox Capture

Copy link
Contributor

@AlbertCarreras AlbertCarreras Oct 3, 2024

Choose a reason for hiding this comment

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

Screenshot by Dropbox Capture
shouldn't this one become selected? when expanded?

Maybe add that comments, when expanded show as selected.

@AlbertCarreras
Copy link
Contributor

There's some shifting here:
Brave Browser - SearchGuide - Gestalt 2024-10-03 at 6 58 02 PM

You can prevent it using a transparent border that sets the space so it's already there and it doens't increase when becomes visible

@@ -0,0 +1,76 @@
import { Avatar, AvatarGroup, Flex, Icon, Image, SearchGuideLink } from 'gestalt';
Copy link
Contributor

@AlbertCarreras AlbertCarreras Oct 3, 2024

Choose a reason for hiding this comment

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

Can we shod all this permutations?

Actually instead of all in one, can we create one example for Avtar, icon and image and have on each the 3 variants? We don't have to enable the selected functionality, just the default visual.

Screenshot by Dropbox Capture
Screenshot by Dropbox Capture
Screenshot by Dropbox Capture
Screenshot by Dropbox Capture

Screenshot by Dropbox Capture

@AlbertCarreras
Copy link
Contributor

All of them look good except the one with the image which has some shifting
Brave Browser - SearchGuide - Gestalt 2024-10-03 at 7 38 15 PM

@AlbertCarreras
Copy link
Contributor

Needs fix.
Brave Browser - SearchGuide - Gestalt 2024-10-03 at 7 42 08 PM

@AlbertCarreras AlbertCarreras self-requested a review October 4, 2024 15:52
@AlbertCarreras AlbertCarreras enabled auto-merge (squash) October 4, 2024 15:53
@AlbertCarreras AlbertCarreras merged commit 199e5a2 into pinterest:master Oct 4, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor release Minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants