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: revamp home ui #334

Merged
merged 21 commits into from
Oct 22, 2024
Merged

Conversation

julia-eileen
Copy link
Contributor

@julia-eileen julia-eileen commented Oct 16, 2024

Acceptance Criteria

  • Configure the project to allow the old UI codebase to coexist with the new one until the revamp is completed.
  • Implement a feature flag system using the useNewUiEnabled hook.
  • Modify the Sass build script to generate output for both the old and new UI.
  • Set up the project to allow a theming system (dark and light mode).
  • Update navigation, status components, and the homepage according to the new UI.

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.
2024-10-16.12-30-01.mp4
2024-10-16.13-39-59.mp4

@julia-eileen julia-eileen requested a review from r4mmer as a code owner October 16, 2024 16:41
@julia-eileen julia-eileen changed the base branch from master to dev October 16, 2024 16:57
@julia-eileen
Copy link
Contributor Author

To quickly test the new UI without modifying the feature flag response, simply update the hooks/useNewUiEnabled.js file with the following content:

/**
 * Copyright (c) Hathor Labs and its affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import { useFlag } from '@unleash/proxy-client-react';
import { UNLEASH_NEW_UI_FEATURE_FLAG } from '../constants';

export const useNewUiEnabled = () => {
  const newUiEnabled = useFlag(UNLEASH_NEW_UI_FEATURE_FLAG) || true;

  return newUiEnabled;
};

@tuliomir tuliomir self-assigned this Oct 16, 2024
@tuliomir tuliomir added the enhancement New feature or request label Oct 16, 2024
Copy link
Contributor

@tuliomir tuliomir left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to our project! Here are some questions and suggestions on an initial partial review.

src/hooks/useIsMobile.js Outdated Show resolved Hide resolved
src/hooks/useNewUiLoad.js Outdated Show resolved Hide resolved
src/utils/getTheme.js Outdated Show resolved Hide resolved
src/hooks/useMount.js Outdated Show resolved Hide resolved
src/screens/DashboardTx.js Outdated Show resolved Hide resolved
src/components/Switch.js Outdated Show resolved Hide resolved
src/components/Switch.js Outdated Show resolved Hide resolved
src/actions/index.js Show resolved Hide resolved
src/components/GDPRConsent.js Outdated Show resolved Hide resolved
src/components/Navigation.js Outdated Show resolved Hide resolved
@tuliomir tuliomir self-requested a review October 21, 2024 14:01
Copy link
Contributor

@tuliomir tuliomir left a comment

Choose a reason for hiding this comment

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

Not related to any file in specific: When running the application in localhost using the new UI, I'm receiving the following warning on browser console:

index.js:26 Warning: validateDOMNesting(...): <li> cannot appear as a descendant of <li>.
    at li
    at ConditionalNavigation (http://localhost:3000/static/js/main.chunk.js:4765:5)

This may be related to some specific usage on the Navigation.js file, but I'm unsure of its source as of now.

@julia-eileen
Copy link
Contributor Author

Not related to any file in specific: When running the application in localhost using the new UI, I'm receiving the following warning on browser console:

index.js:26 Warning: validateDOMNesting(...): <li> cannot appear as a descendant of <li>.
    at li
    at ConditionalNavigation (http://localhost:3000/static/js/main.chunk.js:4765:5)

This may be related to some specific usage on the Navigation.js file, but I'm unsure of its source as of now.

It's because of the dropdown, can we fix it in the next PR? it would take some time to refactor this.

tuliomir
tuliomir previously approved these changes Oct 21, 2024
src/App.js Outdated Show resolved Hide resolved
src/components/Navigation.js Show resolved Hide resolved
src/components/Navigation.js Outdated Show resolved Hide resolved
src/components/Navigation.js Outdated Show resolved Hide resolved
src/components/Navigation.js Show resolved Hide resolved
src/components/Navigation.js Show resolved Hide resolved
src/components/Navigation.js Outdated Show resolved Hide resolved
src/components/Navigation.js Show resolved Hide resolved
src/components/Switch.js Outdated Show resolved Hide resolved
@tuliomir
Copy link
Contributor

Hello @julia-eileen ,
We have a rule in this repository that requires commits made to it to be signed before merging a PR. Could you please sign your commits so that this branch complies with the rule?
Here are some resources to help with that:

If you want, you can also squash all commits into one, so that the signing is made easier.

@julia-eileen julia-eileen force-pushed the feat/revamp-home-ui branch 2 times, most recently from 299bb40 to 50ceb94 Compare October 22, 2024 19:34
@julia-eileen
Copy link
Contributor Author

julia-eileen commented Oct 22, 2024

Hello @julia-eileen , We have a rule in this repository that requires commits made to it to be signed before merging a PR. Could you please sign your commits so that this branch complies with the rule? Here are some resources to help with that:

* GitHub on [signing commits](https://docs.github.com/authentication/managing-commit-signature-verification/about-commit-signature-verification)

* A single command for [signing multiple commits](https://superuser.com/a/1123928), via SO

If you want, you can also squash all commits into one, so that the signing is made easier.

Hi @tuliomir ,

Thank you for pointing that out and for providing the helpful guide! I’ve followed the instructions and signed all of my commits. Please let me know if everything looks good now or if there’s anything else I need to adjust. Thanks again for your support and patience! 🦊

@tuliomir tuliomir merged commit 111a136 into HathorNetwork:dev Oct 22, 2024
1 check passed
@julia-eileen julia-eileen deleted the feat/revamp-home-ui branch October 23, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Waiting to be deployed
Development

Successfully merging this pull request may close these issues.

5 participants