-
Notifications
You must be signed in to change notification settings - Fork 60
Fix header component - Closes #506 #542
Fix header component - Closes #506 #542
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job.
); | ||
|
||
const mapStateToProps = state => ({ | ||
isAuthenticated: !!state.account.publicKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be something else then publicKey
because now it shows the haeder right when I click the "Login" button, but it should only after the server request is finished and it is redirected from login page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently, all private routes look to the same params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In private routes, it makes no user-visible difference. But on the login page if user is on slower network, it can longer time for the request and only then user is redirected from login page. Logout and Send buttons should not be on login page in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixed in the meantime by some other changes.
@@ -39,7 +39,7 @@ class Transactions extends React.Component { | |||
length: parseInt(res.count, 10), | |||
}); | |||
}) | |||
.catch(error => console.error(error.message)); | |||
.catch(error => console.error(error.message)); //eslint-disable-line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that finally somebody got annoyed enough by the warning in console :-) Though I would prefer to disable the specific rule. In this case a believe it is //eslint-disable-line no-console
@@ -16,3 +16,7 @@ | |||
.material-icons{ | |||
font-size: 24px !important; | |||
} | |||
|
|||
.menu { | |||
right: -16px !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be less (maybe right: -8px !important;
) and the put the rest on .wrapper
.
.wrapper
has too much space on the right compared to nano 1.0.0 and not enough at the bottom.
I mean something like this is missing
.wrapper {
margin-right: -8px;
margin-bottom: 16px;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
menu
is a theme param, and it passes to the component and merges with other styles. it is so hard to style Menu
component through IconMenu
component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean .wrapper
which is defined and used in our header
component. Nothing extra in IconMenu
https://github.com/alepop/lisk-nano/blob/2c484cfd1565634b4f682fbf207ee2bc3ce3bb42/src/components/header/header.css#L1-L4
https://github.com/alepop/lisk-nano/blob/2c484cfd1565634b4f682fbf207ee2bc3ce3bb42/src/components/header/headerElement.js#L12-L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do i need to fix it in other pr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please.
<PrivateWrapper />
componentclose Fix Header component #506