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: extend PickerBase component functionality #11851

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

vinnyhoward
Copy link
Contributor

Description

This PR enhances the PickerBase component to provide more flexibility and control to developers, particularly in relation to the dropdown icon styling and positioning. These changes are necessary to accommodate the Header Update design requirements.

Key Changes:

  1. Dropdown Icon Size Control: Developers can now specify the size of the dropdown icon, with a fallback to ensure consistent behavior.

  2. Dropdown Icon Spacing: Added the ability to control the spacing between the dropdown icon and other elements within the component.

  3. Backward Compatibility: These changes are designed to be non-breaking, maintaining compatibility with existing implementations.

Benefits:

  • Increased flexibility for custom designs
  • Better alignment with the new Header Update requirements
  • Improved developer control over component styling

Impact:

Related issues

Related: #11763

Manual testing steps

  1. View any instance of PickerBase being used. For example the NetworkPicker in header in the home screen

Screenshots/Recordings

Before After
original updated

Before

NA

After

NA

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@vinnyhoward vinnyhoward requested a review from a team as a code owner October 17, 2024 22:11
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@vinnyhoward vinnyhoward added team-wallet-ux Run Smoke E2E Triggers smoke e2e on Bitrise team-design-system All issues relating to design system in Mobile labels Oct 17, 2024
Copy link
Contributor

github-actions bot commented Oct 17, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 08f40b3
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bebe9360-dfd3-43e6-8190-82ab40467ffe

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link
Contributor

@brianacnguyen brianacnguyen left a comment

Choose a reason for hiding this comment

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

Question regarding the arrow down icon changes

@vinnyhoward vinnyhoward added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 22, 2024
Copy link
Contributor

github-actions bot commented Oct 22, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: c612d2a
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/df1f0b89-cf1f-43ef-ba8b-e7d896d4066a

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

sonarcloud bot commented Oct 22, 2024

Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

I agree that we should allow access to all child elements within our components. We usually handle this by spreading a prop object to the child components instead of exposing each prop individually, as you're doing with dropdownIconStyle. It might be more flexible to use dropdownIconProps and spread them to the <Icon {...dropdownIconProps} />, allowing props to be overridden, but that would require somewhat complex types which is out of the scope of this PR. Approving to unblock.

@vinnyhoward
Copy link
Contributor Author

I agree that we should allow access to all child elements within our components. We usually handle this by spreading a prop object to the child components instead of exposing each prop individually, as you're doing with dropdownIconStyle. It might be more flexible to use dropdownIconProps and spread them to the <Icon {...dropdownIconProps} />, allowing props to be overridden, but that would require somewhat complex types which is out of the scope of this PR. Approving to unblock.

That makes sense. I can do a follow up after the header redesign is finished to allow that. Thanks for reviewing and approving🫡 @georgewrmarshall

@vinnyhoward vinnyhoward added this pull request to the merge queue Oct 24, 2024
Merged via the queue into main with commit 6b2cf82 Oct 24, 2024
43 checks passed
@vinnyhoward vinnyhoward deleted the feat-extend-pickerbase-functionality branch October 24, 2024 00:18
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
@metamaskbot metamaskbot added the release-7.35.0 Issue or pull request that will be included in release 7.35.0 label Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.35.0 Issue or pull request that will be included in release 7.35.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-design-system All issues relating to design system in Mobile team-wallet-ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants