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

Implement new design of main header - Closes #2053 #2120

Open
wants to merge 40 commits into
base: development
from

Conversation

2 participants
@massao
Copy link
Contributor

commented Jun 6, 2019

What issue have I solved?

#2053

How have I implemented/fixed it?

  • Refactored some toolbox components to be more customizable (Icons, OutsideClickHandler);
  • Added Separator component to DropdownV2;
  • Refactored elements from topBar to be easier to apply the final UI;
  • Updated account dropdown with token switcher;
  • Added network status to topBar;
  • Updated e2e tests due to changes on what informations are displayed on the top bar and how they are displayed;
  • Fix issue with settings reducer not enhancing the initial state with the localStorage, but replacing it instead;
  • Fix bug on OutsideClickHandler that wasn't unbind properly;
  • Updated hover transaction table row color;
  • The network status is show even on guest mode so it's possible to know which network it's connected to;

How has this been tested?

  • Check that all the functionalities are working as expected;
  • Add btc to localStorage: localStorage.setItem('btc', true);
  • Check that it's possible to switch tokens through the dropdown menu;
  • Check smaller resolutions;

Review checklist

massao added some commits Jun 6, 2019

@massao massao added this to the 1.20.0 milestone Jun 6, 2019

@massao massao self-assigned this Jun 6, 2019

@massao massao added this to Pull Requests in Version 1.19.0 via automation Jun 6, 2019

massao added some commits Jun 6, 2019

@massao massao force-pushed the 2053-implement-new-design-of-main-header branch from 954c30e to da72d66 Jun 12, 2019

@massao massao force-pushed the 2053-implement-new-design-of-main-header branch from 3a5c817 to 733a92c Jun 12, 2019

massao added some commits Jun 12, 2019

@massao massao requested a review from osvaldovega Jun 13, 2019

@massao massao marked this pull request as ready for review Jun 13, 2019

massao added some commits Jun 14, 2019

@osvaldovega
Copy link
Contributor

left a comment

great man a few comments but not big deal...

@@ -12,6 +12,6 @@

@media (--small-viewport) {

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Jun 14, 2019

Contributor

You can remove this media query you dont need it

@@ -120,11 +116,11 @@ or "warn/action" ineastd of "red/green"
--color-content-grayblue: #7383a7;
--color-background-wrapper: var(--color-mystic);
--color-background-header: #fbfcfd;
--color-background-menu: #f8f9fb;
--color-background-menu: var(--color-white-smoke);

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Jun 14, 2019

Contributor

I dont know what do you think but for avoid more variables maybe you can remove this line and instead use --color-background-menu you can use --color-white-smoke

This comment has been minimized.

Copy link
@massao

massao Jun 14, 2019

Author Contributor

As discussed, we this variable is being used in other components so we should remove them later, since they are outside the scope of the project, and it's just to keep the same colors across the application

--color-link-active: #4070f4;
--color-title-header-box: #4c4c4c;
--color-link: var(--color-primary-standard);
--color-rows-hover: rgba(157, 184, 250, 0.2);
--color-rows-hover: var(--primary-background-color);

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Jun 14, 2019

Contributor

same here

This comment has been minimized.

Copy link
@massao

massao Jun 14, 2019

Author Contributor

As discussed, we this variable is being used in other components so we should remove them later, since they are outside the scope of the project, and it's just to keep the same colors across the application

@@ -20,7 +21,8 @@ class SearchBar extends React.Component {
};

this.onChangeSearchTextValue = this.onChangeSearchTextValue.bind(this);
this.onSelectedRow = this.onSelectedRow.bind(this);
this.onSelectAccount = this.onSelectedRow.bind(this, 'accounts');
this.onSelectTransaction = this.onSelectedRow.bind(this, 'transactions');
this.clearSearch = this.clearSearch.bind(this);
this.isSubmittedStringValid = this.isSubmittedStringValid.bind(this);

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Jun 14, 2019

Contributor

You can remove this lines from constructor you dont need it.

this.clearSearch = this.clearSearch.bind(this);
this.isSubmittedStringValid = this.isSubmittedStringValid.bind(this);
this.onKeyPressDownOrUp = this.onKeyPressDownOrUp.bind(this);
this.onKeyPress = this.onKeyPress.bind(this);

Then you can remove in line 13 the eslint
// eslint-disable-next-line max-statements

}

if (suggestions.transactions.length) {
this.onSelectedRow(suggestions.transactions[rowItemIndex].id, 'transaction');
this.onSelectTransaction(suggestions.transactions[rowItemIndex].id);

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Jun 14, 2019

Contributor

What do you think about refactor this function onKeyPress like this. To avoid repeat many times suggestions

onKeyPress() {
    const { suggestions: { addresses, delegates, transactions } } = this.props;
    const { rowItemIndex } = this.state;

    if (addresses.length) this.onSelectAccount(addresses[rowItemIndex].address);
    if (delegates.length) this.onSelectAccount(delegates[rowItemIndex].account.address);
    if (transactions.length) this.onSelectTransaction(transactions[rowItemIndex].id);
  }
@@ -32,13 +34,14 @@ class NavigationButtons extends React.Component {
});
}

// eslint-disable-next-line max-statements

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Jun 14, 2019

Contributor

I think you can remove this line

@massao massao removed this from the Sprint 2 milestone Jun 14, 2019

massao added some commits Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.