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

⚙️ Add the first page for the menu/settings tab #342

Merged
merged 6 commits into from
Oct 19, 2021
Merged

Conversation

henryboldi
Copy link
Contributor

@henryboldi henryboldi commented Oct 19, 2021

Settings tab

This aims to eventually allow users to edit extension-wide settings. Currently, this is all just visual components on the first page. That said, the toggle button does flip between off and on when clicked with a smooth transition.

Settings

A WIP. I'd like to split out some of these pieces into their own component file. And this is all just visual.
Originally was in the Menu page itself
@henryboldi henryboldi marked this pull request as ready for review October 19, 2021 03:35
@henryboldi henryboldi requested a review from a team October 19, 2021 03:37
Copy link
Contributor

@itsrachelfish itsrachelfish left a comment

Choose a reason for hiding this comment

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

Tested locally and it looks good! But I think this could use a couple minor code changes

Copy link
Contributor

@greg-nagy greg-nagy left a comment

Choose a reason for hiding this comment

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

nice PR and I dig the working toggle button w/ transition :)

had one question about the settings array in the Menu.tsx

<div className="icon_lock" />
<h3>General</h3>
<ul>
{settings.slice(0, 4).map((setting) => {
Copy link
Contributor

@greg-nagy greg-nagy Oct 19, 2021

Choose a reason for hiding this comment

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

Is this a common pattern in react?
It might be easier to follow using the values inline in the template instead of using settings array and slice on it.

The Main Currency action might be a good candidate for putting it in a separate function though

henryboldi and others added 2 commits October 19, 2021 12:59
Co-authored-by: Rachel Fish <itsrachelfish@gmail.com>
Co-authored-by: Rachel Fish <itsrachelfish@gmail.com>
Copy link
Contributor

@itsrachelfish itsrachelfish 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 👍

@itsrachelfish itsrachelfish merged commit 284efed into main Oct 19, 2021
@itsrachelfish itsrachelfish deleted the settings-tab branch October 19, 2021 18:31
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.

3 participants