Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Home landing page #4178

Merged
merged 150 commits into from
Feb 14, 2017
Merged

Home landing page #4178

merged 150 commits into from
Feb 14, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Jan 16, 2017

Actions -

  1. Created views/Home that shows the actual components (urls, dapps, news & accounts - all historic)
  2. Add /home route & menubar item (replacing previous non-clickable image). This is now the default landing page.

parity 2017-01-26 11-39-14

In action -

https://youtu.be/eGngA2lj3Gk

Additional required dependencies -

  1. *.web.web3.site & history from Parity Node, Persistent tracking of dapps #4302 & https://github.com/ethcore/parity/tree/ethlink
  2. Add utils/dapplink to encode <token+url>.web.web3.site URLs, Web view with web3.site support #4313
  3. Split DappUrlInput component from views/Web, allowing the re-usable entry of a web URL, Web view with web3.site support #4313
  4. Split logic from views/Web into re-usable store, Web view with web3.site support #4313
  5. Split Tabbar Tab component into separate file, Split Tab from TabBar #4318

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Jan 16, 2017
@jacogr jacogr changed the title Jg home Home landing page Jan 16, 2017
import base32 from 'base32.js';

// base32 alphabet should match the Rust implementation
// https://github.com/andreasots/base32/blob/master/src/base32.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to have a non-volatile link to a specific commit in here.

Copy link
Contributor Author

@jacogr jacogr Jan 18, 2017

Choose a reason for hiding this comment

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

We are not even close to having that - we are still actually in development here and finding what works between the platforms without decoding issues.

(At this point is a align-tests between platforms, push, build binaries and test exercise - that comment is due to "tracking between days" to not lose context.)

@jacogr
Copy link
Contributor Author

jacogr commented Feb 13, 2017

Updated.

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 13, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Feb 13, 2017

The dapps and the accounts looks very similar and I think it would be easier if it were one component. The link and the title should be the only thing that differ, as well as the image. No ?

That's when the cursor is over the dapps zone, but not over a dapp

image

That's when the cursor is outside the dapps zone

image

@ngotchac
Copy link
Contributor

When the node is not connected, it shows the accounts as unnamed

image

>
<Link
className={ styles.link }
to={ `/${history.type === 'wallet' ? 'wallet' : 'accounts'}/${history.entry}` }
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to connect this component to the Redux state, so it can directly check if it's an Account, a Wallet or an address. The link URL would be generated accordingly.
The point being that if I visit an Account and then delete it, it would still show up in the history and the link would be broken. The parent component could be connected not to render this deleted Account though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 Good point.

body: item.style ? (item.style.body || {}) : {},
head: item.style ? (item.style.head || {}) : {},
tags: item.style ? (item.style.tags || {}) : {}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply { ...item.style } ? Could filter which key with lodash/omitBy if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is due to evolution over time and figuring out what makes sense. { ...item.style } would be preferable.

);
}

retrieveNews () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have a simple MobX store for that ? Would be great to have it as a singleton to be able to cache the result (and update if necessary). When you switch back to the home page without leaving the UI, there is always at least 1 sec without the news displayed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, been to-and-fro-ing this one in my head. Feels like overkill (store for 1 call), however the singleton actually does make sense here, just because of the delay. Will update.

return (
<SectionList
className={ styles.news }
items={ newsItems }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it limited to 3 news ?

Copy link
Contributor Author

@jacogr jacogr Feb 13, 2017

Choose a reason for hiding this comment

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

More sections goes into the next line, so there can be as many as we want.

if (extensionStore.hasExtension) {
window.open(this.props.store.currentUrl, '_blank');
} else {
router.push('/web');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be a proper a link instead ? As it's just a link, using the standard attributes sounds a bit more logical (just thinking about users with no extension that want to ctrl+click on the link to open the dapp in a new tab)

Copy link
Contributor Author

@jacogr jacogr Feb 13, 2017

Choose a reason for hiding this comment

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

It becomes quite hairy and very messy prior to <base36>.web.web3.site support for the internal dapps -

  1. Internal vs external to use Link (internal) and a (external)
  2. Still need to set set the currentUrl in the Webstore before opening /web

Could probably re-look at the implementation with an issue, here though I am not going to touch everything down from Web/Store and re-imagine the world. So I would suggest logging as an enhancement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes ok, I haven't seen that the Browser dapp don't take into account the real browser url.

@ngotchac ngotchac added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 13, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 13, 2017

Updated.

  • UI overlaps, pulled in my changes from the Vaults branch re Container.
  • Upon connecting, doesn't show Home page
  • News into seperate singleton store
  • Items styles now expanded inline
  • Auto-detection of link type based on Redux info

As for similar components, in isolation, yes. However these would need to be replaced by standard summary components as soon as we get to the Accounts/Address/Contract/Dapp overlay-change PRs. (So really don't want to add generic component work here to turf it 3 PRs down the line especially without seeing what the needs are across views.)

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 13, 2017
Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

Okay, looks good now. The only thing is when you click on a dapp, and then on Parity to go back to the UI, it navigates to the dapp view, instead of the previous home view

@ngotchac ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 13, 2017
@jacogr jacogr merged commit 63d2cfc into master Feb 14, 2017
@jacogr jacogr deleted the jg-home branch February 14, 2017 07:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants