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

make channel names on discover page links #869

Merged
merged 1 commit into from Dec 14, 2017

Conversation

Projects
None yet
5 participants
@daovist
Copy link
Contributor

daovist commented Dec 13, 2017

No description provided.

@daovist

This comment has been minimized.

Copy link
Contributor Author

daovist commented Dec 13, 2017

lbry_pr_869

@liamcardenas liamcardenas requested a review from seanyesmunt Dec 13, 2017

@liamcardenas

This comment has been minimized.

Copy link
Contributor

liamcardenas commented Dec 13, 2017

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?

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Dec 13, 2017

Thanks for the contribution @daovist!

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

@seanyesmunt
Copy link
Member

seanyesmunt left a comment

Small issue

@@ -89,25 +89,37 @@ class FileCard extends React.PureComponent {
className="card__link"
>
<CardMedia title={title} thumbnail={thumbnail} />
<div className="card__title-identity">
</Link>

This comment has been minimized.

@seanyesmunt

seanyesmunt Dec 13, 2017

Member

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

This comment has been minimized.

@daovist

daovist Dec 13, 2017

Author Contributor

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.

This comment has been minimized.

@daovist

daovist Dec 13, 2017

Author Contributor

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

This comment has been minimized.

@seanyesmunt

seanyesmunt Dec 13, 2017

Member

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.

@tzarebczan

This comment has been minimized.

Copy link
Member

tzarebczan commented Dec 13, 2017

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

Thanks again for your contribution!

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Dec 13, 2017

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

This comment has been minimized.

Copy link
Member

kauffj commented Dec 13, 2017

@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

This comment has been minimized.

Copy link
Member

seanyesmunt commented Dec 13, 2017

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

This comment has been minimized.

Copy link
Contributor Author

daovist commented Dec 13, 2017

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.

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Dec 13, 2017

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();
}
@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Dec 13, 2017

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

@daovist

This comment has been minimized.

Copy link
Contributor Author

daovist commented Dec 14, 2017

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.

@@ -44,6 +46,20 @@ const Link = props => {
);
}

if (span === true) {

This comment has been minimized.

@liamcardenas

liamcardenas Dec 14, 2017

Contributor

this can just be if(span) {

</span>
);
}

return (
<a

This comment has been minimized.

@liamcardenas

liamcardenas Dec 14, 2017

Contributor

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!

const { claim, link, uri, isResolvingUri, smallCard } = this.props;
const { claim, link, uri, isResolvingUri, smallCard, span } = this.props;

console.log("URIINDICATOR SPAN:", span);

This comment has been minimized.

@liamcardenas

liamcardenas Dec 14, 2017

Contributor

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

@seanyesmunt
Copy link
Member

seanyesmunt left a comment

One issue then I will merge it

@@ -91,6 +91,7 @@ class UriIndicator extends React.PureComponent {
navigate="/show"
navigateParams={{ uri: channelLink }}
className="no-underline"
span={span ? span : {}}

This comment has been minimized.

@seanyesmunt

seanyesmunt Dec 14, 2017

Member

You can just pass this in as is
span={span}

If it is undefined that is ok

@seanyesmunt

This comment has been minimized.

Copy link
Member

seanyesmunt commented Dec 14, 2017

Could you also squash these commits into one commit?

@daovist daovist force-pushed the daovist:master branch from d1b6f77 to ca527cc Dec 14, 2017

@seanyesmunt seanyesmunt merged commit d1e0f9d into lbryio:master Dec 14, 2017

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.