LBRYCommunity/lbry-desktop

Fork 0

make channel names on discover page links #869

Merged

daovist merged 1 commit from master into master

2017-12-14 22:15:14 +01:00

daovist commented

2017-12-13 20:52:47 +01:00

(Migrated from github.com)

No description provided.

daovist commented

2017-12-13 21:04:28 +01:00

(Migrated from github.com)

![lbry_pr_869](https://user-images.githubusercontent.com/34498824/33959877-e702645e-e016-11e7-8028-3e534a8a2f92.png)

liamcardenas commented

2017-12-13 21:11:38 +01:00

(Migrated from github.com)

Thanks @daovist! @seanyesmunt can you review this to make sure it is up to your UI standards? also, the @stealthishow is now green because it is a link, do you think we should keep it that way or change it back to gray?

Thanks @daovist! @seanyesmunt can you review this to make sure it is up to your UI standards? also, the `@stealthishow` is now green because it is a link, do you think we should keep it that way or change it back to gray?

neb-b commented

2017-12-13 21:25:17 +01:00

(Migrated from github.com)

Thanks for the contribution @daovist!

@liamcardenas I think the green color is good, it looks the same as on the file page.

Thanks for the contribution @daovist! @liamcardenas I think the green color is good, it looks the same as on the file page.

neb-b (Migrated from github.com) requested changes 2017-12-13 21:27:18 +01:00

neb-b (Migrated from github.com) left a comment

Small issue

src/renderer/component/fileCard/view.jsx

neb-b (Migrated from github.com) commented

2017-12-13 21:27:05 +01:00

Instead of closing the link here, does it work if you keep it at the bottom and encapsulate the new link? You should still be able to click on the file if your mouse is below the channel name.

Hope that makes sense

Instead of closing the link here, does it work if you keep it at the bottom and encapsulate the new link? You should still be able to click on the file if your mouse is below the channel name. Hope that makes sense

daovist (Migrated from github.com) reviewed 2017-12-13 21:40:00 +01:00

src/renderer/component/fileCard/view.jsx

daovist (Migrated from github.com) commented

2017-12-13 21:40:00 +01:00

That was my first thought too. In that case it takes you to the file page and not the channel page. Though I think it actually takes you to the channel page and then the file page (calling the navigate function twice?) because when I click the channel link and then click Back it does take me to the channel page.

daovist (Migrated from github.com) reviewed 2017-12-13 21:41:37 +01:00

src/renderer/component/fileCard/view.jsx

daovist (Migrated from github.com) commented

2017-12-13 21:41:37 +01:00

I ended up wrapping links around the image, title, and price to avoid this.

tzarebczan commented

2017-12-13 21:57:27 +01:00

(Migrated from github.com)

@daovist - also be sure to check out https://lbry.io/faq/tips

Thanks again for your contribution!

@daovist - also be sure to check out https://lbry.io/faq/tips Thanks again for your contribution!

neb-b (Migrated from github.com) reviewed 2017-12-13 22:02:25 +01:00

src/renderer/component/fileCard/view.jsx

neb-b (Migrated from github.com) commented

2017-12-13 22:02:25 +01:00

You should be able to use e.stopPropagation to fix that. We still want to keep just one link for the whole card. You could update the <Link /> component to take a prop noPropagation which would call e.stopPropagation() before doing the navigate.

I think that would work.

You should be able to use `e.stopPropagation` to fix that. We still want to keep just one link for the whole card. You could update the `<Link />` component to take a prop `noPropagation` which would call `e.stopPropagation()` before doing the navigate. I think that would work.

neb-b commented

2017-12-13 22:09:49 +01:00

(Migrated from github.com)

Actually I am seeing that nested links are illegal in HTML5. I just tested and my idea works, but I see an ugly error.

<a> cannot appear as a descendant of <a>

@kauffj thoughts?

I think the larger issue is that internal links should just be buttons (which I am going to do for the redesign).

Actually I am seeing that nested links are illegal in HTML5. I just tested and my idea works, but I see an ugly error. ``` <a> cannot appear as a descendant of <a> ``` @kauffj thoughts? I think the larger issue is that internal links should just be buttons (which I am going to do for the redesign).

kauffj commented

2017-12-13 22:48:12 +01:00

(Migrated from github.com)

@seanyesmunt Not sure of another solution to the nested anchor issue other than just attaching an onClick to a non-anchor element like a <div>. I believe we ran into this on <FileTile> and that's what it does.

@seanyesmunt Not sure of another solution to the nested anchor issue other than just attaching an `onClick` to a non-anchor element like a `<div>`. I believe we ran into this on `<FileTile>` and that's what it does.

neb-b commented

2017-12-14 00:08:21 +01:00

(Migrated from github.com)

I think that would be fine for now. @daovist could you try replacing the wrapper <Link /> to a div and moving the onClick to that? Then you would be able to add the link to the channel no problem. (you would still need to add the e.stopPropagation

I think that would be fine for now. @daovist could you try replacing the wrapper `<Link />` to a div and moving the `onClick` to that? Then you would be able to add the link to the channel no problem. (you would still need to add the `e.stopPropagation`

daovist commented

2017-12-14 00:35:01 +01:00

(Migrated from github.com)

I'm trying. The URI it gives me is for the video and I haven't figured out how to get the channel link the way the UriIndicator component does. I'm messing around with lbryuri.js but not having any luck so far.

neb-b commented

2017-12-14 00:44:17 +01:00

(Migrated from github.com)

You should still use the UriIndicator component. Just change the highest level wrapper <Link to a div.

If you still the the navigate action being fired twice you can change the <Link component to use event.stopPropagation()

Maybe something like this

// in UriIndicator/view.jsx
<Link noPropagation />


// inside link/view.jsx
const onClick = (e) => {
  if (props.noPropagation) {
    e.stopPropagation();
  }
  props.onClick();
}

You should still use the `UriIndicator` component. Just change the highest level wrapper `<Link` to a `div`. If you still the the `navigate` action being fired twice you can change the `<Link` component to use `event.stopPropagation()` Maybe something like this ``` // in UriIndicator/view.jsx <Link noPropagation /> // inside link/view.jsx const onClick = (e) => { if (props.noPropagation) { e.stopPropagation(); } props.onClick(); } ```

neb-b commented

2017-12-14 00:45:16 +01:00

(Migrated from github.com)

You can message me on discord if that doesn't work, a little easier to chat there.

daovist commented

2017-12-14 18:41:07 +01:00

(Migrated from github.com)

Nested anchors work but are not legit HTML. When I replace anchor with a span or div it messes up a lot of things throughout LBRY, such as FAQ and TOS links, not to mention navigation. So I added a span value to props which if true returns a span instead of an anchor which I am using to link to the channel pages of videos on the Discover page.

liamcardenas (Migrated from github.com) requested changes 2017-12-14 20:46:28 +01:00

src/renderer/component/link/view.jsx

liamcardenas (Migrated from github.com) commented

2017-12-14 20:38:25 +01:00

this can just be if(span) {

this can just be `if(span) {`

liamcardenas (Migrated from github.com) commented

2017-12-14 20:42:49 +01:00

this whole thing should probably be structured like this

let props = { className: combinedClassName, href: href || "javascript:;" etc}

and then the if statement should be something like
return span ? <span {...props}> {content} </span> : <a {...props}> {content} </a>

as per https://zhenyong.github.io/react/docs/jsx-spread.html

let me know if you have any questions!

this whole thing should probably be structured like this `let props = { className: combinedClassName, href: href || "javascript:;" etc}` and then the if statement should be something like `return span ? <span {...props}> {content} </span> : <a {...props}> {content} </a>` as per https://zhenyong.github.io/react/docs/jsx-spread.html let me know if you have any questions!

src/renderer/component/uriIndicator/view.jsx

liamcardenas (Migrated from github.com) commented

2017-12-14 20:43:32 +01:00

whenever you are ready to merge, be sure to remove this ;)