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 bookmarks component in dashboard - closes #2043 #2087

Conversation

Projects
None yet
2 participants
@osvaldovega
Copy link
Contributor

commented Jun 3, 2019

What issue have I solved?

-- #2043

How have I implemented/fixed it?

This PR contains the new design for the Bookmarks component in the dashboard page.
The new behavior is based on the active token, so if the user has selected LSK as active token for the app, thee bookmarks will show the last 5 bookmarks, if the user switch to BTC, then will show the BTC bookmarks.

In case of no saved bookmarks and empty state will be display.

How has this been tested?

For see this dashboard with the new bookmarks design, please log in into the app and go to dashboard v2, ilke this.

1-. /#/dashboardV2
2. Then you will see the list of bookmarks in case of have it.
3. If there is no bookmarks, then save some in the send component or wallet component and go back to dashboardV2
4. For switch to BTC a action needs to be dispatch in the redux development tool in the browser.
For LSK

{ type: 'SETTINGS_UPDATED', data: { token: { active: 'LSK' } } }

For BTC

{ type: 'SETTINGS_UPDATED', data: { token: { active: 'BTC' } } }

NOTE: The bookmark main page it is not implemented so the view all button at this point it is not redirecting to the correct page until this be implemented, once this be implemented the view all button will redirect to this page, same as the empty state.

<div>
  <Link to={'#'}>
    <SecondaryButtonV2>{t('Search Accounts')}</SecondaryButtonV2>
  </Link>
</div>

and

<Link to={'#'}>
  <SecondaryButtonV2>{t('View All')}</SecondaryButtonV2>
</Link>

Review checklist

osvaldovega added some commits May 31, 2019

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

@osvaldovega osvaldovega self-assigned this Jun 3, 2019

@massao
Copy link
Contributor

left a comment

Some comments, when viewing BTC bookmarks, the spacing is wrong, probably because it doesn't have the avatar:
image

import { BrowserRouter as Router } from 'react-router-dom';
import BookmarksList from './bookmarksList';
import { tokenMap } from '../../constants/tokens';
// import keyCodes from './../../constants/keyCodes';

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

Remove commented code.


.tab {
border-bottom: none !important;
}

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

This can be removed since we don't use the tabs anymore


.box {
width: 100%;
max-width: 321px;

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

Wouldn't be better to have the sizing on the parent component? So even if this component should be used in a different size in the future it's easier.

const { bookmarks, token } = this.props;

const actualBookmarks = { ...bookmarks };
return actualBookmarks[token.active].length > 5

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

Don't need to check the length of the bookmarks here, you can just return bookmarks[token.active].slice(0, 5);

<h1>{t('No Bookmarks added yet')}</h1>
<p>{t('Start adding some addresses to bookmarks, to keep track of them.')}</p>
<div>
<Link to={'#'}>

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

I would add a // TODO comment to be easier to find this on the code.

{
selectedBookmarks.length
? <div className={styles.footer}>
<Link to={'#'}>

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

Would add a // TODO comment here too

BookmarksList.propTypes = {
bookmarks: PropTypes.object.isRequired,
history: PropTypes.object.isRequired,
};

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

Since the props come from the redux store, I'm not sure if it's needed to put them at propTypes, since it wouldn't be possible to not pass them to the component.

<p>{t('A great way to start is to top up your account with some {{value}} tokens.', { value: activeToken.key })}</p>
<div>
{
activeToken.key === 'LSK' // TODO this validation should be remove once we have the external link for BTC

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

Should be using tokenMap constant

<div
key={bookmark.address}
className={`${styles.row} bookmark-list-row`}
onClick={() => this.onBookmarkSelected(bookmark.address)}>

This comment has been minimized.

Copy link
@massao

massao Jun 4, 2019

Contributor

Isn't it possible to instead of having the onClick, to do a history.push, wrap this inside a <Link>?

@massao

massao approved these changes Jun 4, 2019

Copy link
Contributor

left a comment

👍

@osvaldovega osvaldovega merged commit 113ec61 into development Jun 4, 2019

3 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@osvaldovega osvaldovega deleted the 2043-implement-new-design-of-bookmarks-component-in-dashboard branch Jun 4, 2019

@yasharAyari yasharAyari changed the title Implement new design of bookmarks component in dashboard - closes 2043 Implement new design of bookmarks component in dashboard - closes #2043 Jun 5, 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.