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

Refactor Menu #715

Merged
merged 7 commits into from
Jun 21, 2022
Merged

Refactor Menu #715

merged 7 commits into from
Jun 21, 2022

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Jun 20, 2022

Summary

@fairlighteth instead of adding a lot of feedback in your PR I preferred to reiterate in your PR and document exactly what I changed and why. Sorry if it's too detailed

This PR is an attempt to refactor and cleanup further of #707

The logic is maintained, but I just rewrote all types, and restructure the code to try to make it more maintainable.

Some of the changes in style were:

  • Changed many names.
    • Components must start with capital letter
    • This get functions are in reality a component. So they also should be capitalised.
      i.e before , after )
    • We use a naming convention for the props <Name-of-component>Props. For example before , after
  • Logic was still a bit overcomplicated. i.e. before, after
  • Drill down props were encapsulated in an object, so the props can be simpler, and is easier to drill down the props. i.e. before before, after

Also MAIN_MENU_TYPE because is a type shouldn't be written in all capitals. It was also broken down to be able to define more semantic types and reuse for both the data definition and the components. For example, now the new types can be both be used in the data definition (so they are checked), and also in they components, like this DropDown can receive a DropDownItem and render.

Breaking down the types also allowed to model as mandatory some fields that before where optional i.e. this title is not optional anymore.

The external/internal logic is better handled, instead of relying on a boolean and then adding this logic which can be complex to follow, because reports duplicated fields like onClick and onClickOptional. Now relies in types so representing the internal/external link is simpler.

A similar thing was done for the Dark Mode button. Instead of relying on some String action which makes the component too hard to maintain, we rely on types so rendering this special link is simple

@github-actions
Copy link
Contributor

CLA Assistant Lite All Contributors have signed the CLA.

@anxolin anxolin changed the title Menu improvements2 Refactor Menu Jun 20, 2022
@anxolin anxolin changed the base branch from menu-improvements to develop June 20, 2022 21:22
@anxolin anxolin requested review from fairlighteth and a team June 20, 2022 22:07
@anxolin anxolin marked this pull request as ready for review June 20, 2022 22:07
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

Copy link
Collaborator

@alfetopito alfetopito 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, works fine.

I'm just confused by one interface

src/custom/constants/mainMenu.ts Show resolved Hide resolved
@fairlighteth fairlighteth merged commit cd55b05 into develop Jun 21, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2022
@alfetopito alfetopito deleted the menu-improvements2 branch June 21, 2022 10:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants