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: create new dropdown menu #666

Merged
merged 5 commits into from
Apr 13, 2023
Merged

feat: create new dropdown menu #666

merged 5 commits into from
Apr 13, 2023

Conversation

ckniffen
Copy link
Collaborator

@ckniffen ckniffen commented Apr 12, 2023

High Level Overview of Change

Define <Dropdown> and update <BalanceSelector> to use it.

Removing nested menu code because it is not used with current navigation and while support was defined for the desktop menu it was not for mobile in addition to the styles being very off on the desktop.

Future PRs will use in the header too. This will eliminate 3 menu implementations.

Context of Change

Related to #554 and a new header design.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates

TypeScript/Hooks Update

  • Updated files to React Hooks
  • Updated files to TypeScript

Before

Balance Selector - Before

After

Balance Selector - After

Future Tasks

Use new component in the header

@ckniffen ckniffen requested review from justinr1234 and pdp2121 April 12, 2023 17:38
src/containers/Accounts/AccountHeader/BalanceSelector.tsx Outdated Show resolved Hide resolved
src/containers/Accounts/AccountHeader/BalanceSelector.tsx Outdated Show resolved Hide resolved
src/containers/shared/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
const globalClickListener = useCallback((nativeEvent) => {
// ignore click event happened inside the dropdown menu
if (dropdownRef.current && dropdownRef.current.contains(nativeEvent.target))
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

I’d suggest changing eslint (in another ticket) to not allow this style of return without curly braces.

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 could not find the rule. Do you know which one it might be?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Define `<Dropdown>` and update `<BalanceSelector` to use it.

Removing nested menu code because it is not used with current navigation
and while support was defined for the desktop menu it was not for mobile
in addition to the styles being very off on the desktop.

Future PRs will use <Dropdown> in the header too. This will eliminate 3
menu implementations.
@ckniffen ckniffen force-pushed the feature/new-dropdown branch from 2086633 to 7c1a92c Compare April 13, 2023 00:49
Copy link

@xrpl-grants xrpl-grants left a comment

Choose a reason for hiding this comment

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

LGTM other than Justin's comments for the future

@ckniffen ckniffen mentioned this pull request Apr 13, 2023
@ckniffen ckniffen merged commit ca55ff8 into staging Apr 13, 2023
@ckniffen ckniffen deleted the feature/new-dropdown branch April 13, 2023 16:34
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.

4 participants