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

Differenciate desktop and mobile to clean up tests #465

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Dec 12, 2023

closes #447


Submission checklist:

Layout

  • Change inspected in the desktop web ui
  • Change inspected in the mobile web ui

@@ -27,7 +28,7 @@ const isOptionEqualToValue = (option?: MultiProxy, value?: MultiProxy) => {
return option.multisigs[0].address === value.multisigs[0].address
}

const MultiProxySelection = ({ className }: Props) => {
const MultiProxySelection = ({ className, isDesktop }: Props) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably use the hook, to avoid passing the props to the component. F.e

const isDesktop = useMediaQuery((theme: Theme) => theme.breakpoints.up('md'))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this hook should be used here. At the end of the day, it's just a testId to identify the specific MultiProxySelection component. The fact that we're using it for dektop or mobile is not relevant.

I'll rename it in testId.

Copy link
Collaborator

@Lykhoyda Lykhoyda left a comment

Choose a reason for hiding this comment

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

Looks good! There is a suggestion about isDesktop variable

Copy link
Member

@asnaith asnaith 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, thanks!

@Tbaut Tbaut merged commit 10c7e42 into main Dec 12, 2023
6 checks passed
@Tbaut Tbaut deleted the tbaut-multiproxy-selector-desktop branch December 12, 2023 18:18
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.

Use unique element locators for multiproxy selector on desktop and mobile
3 participants