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

Internal link component [Issue #1737] #1753

Merged
merged 12 commits into from
Nov 12, 2020
Merged

Internal link component [Issue #1737] #1753

merged 12 commits into from
Nov 12, 2020

Conversation

wackerow
Copy link
Member

@wackerow wackerow commented Nov 6, 2020

Description

First draft: This adds a component, currently called DocLink that takes props to, title and description and renders a styled(Link) container with the title/description.

Screenshots with light/dark mode, and link hover-over

image
image
(View Netlify for this PR: /en/glossary/)

To discuss/decide/do

  • Need to decide how long we're willing to let the description be. Could overshoot and then limit to number of lines and truncate, or could just manually decide how much and what info to put in each one (I erred on the side of putting too much in and in generally copies the first paragraph of each page).
  • Some small design related tweaks I was playing around with and settled on this as a first suggestion. I tried the shadow-box approach first and it felt like the component overshadowed the actual glossary entires. Found that border of 2px without the shadow, with a color change and very slight shadow on hover-over felt nice, but this is obviously not my decision to make, just wanted to offer some thoughts and we can go from here.
  • Consider changing name of component

Note- This was created as a branch off dev prior to the other glossary update PRs #1724 and #1725 being merged.

Labeled this as draft til we sort out above details, and figured after the other glossary PRs merge in I can fix any conflicts that this causes, and fix any terms missing here. Wanted to get something up for you guys to be able to review though, let me know what you think.

Related Issue

#1737

@github-actions github-actions bot added Status: Review Needed content 🖋️ This involves copy additions or edits labels Nov 6, 2020
@ryancreatescopy
Copy link
Contributor

Tried to commit a design alternative but not sure it worked...

Bearing in mind, we want to make sure it's clear to users that they're navigating to another page within the current site but we don't want the design to dominate the glossary entries.

image
image
image
image

Regarding the copy length... I'm against imposing strict character limits or truncation. Instead we may just need to be very careful to craft as short a description as we can that is still helpful. I can definitely help with this.

wackerow and others added 2 commits November 6, 2020 10:59
Co-Authored-By: ryancreatescopy <ryancordell@virginmedia.com>
On mobile screens, large links without wrap-points are forcing layout to be too wide for screen. This shortens the link names being rendered to only include the last folder name of the path (ie. `/en/developers/docs/accounts/#types-of-account` will only show `/accounts/` on mobile)
@wackerow
Copy link
Member Author

wackerow commented Nov 6, 2020

Screenshots as of b458d28

image
image
image
image

@samajammin @ryancreatescopy This may be ready for review if you're comfortable with the length of the descriptions being used (this could always be addressed later in another PR)

@ryancreatescopy
Copy link
Contributor

Just want to throw some alternatives into the mix. Do you think we need to actually show a page description? Or should the title be descriptive enough for these purposes? Have we overthought this and perhaps it'd just be simpler to do emoji + title + arrow...

Just wanted to raise these before we decide and merge something...

image
image
image

@wackerow
Copy link
Member Author

wackerow commented Nov 9, 2020

@ryancreatescopy Hm...
So as I was putting this together I did feel that several link "descriptions" were... semi redundant? Especially in cases where the information was almost the same, it seemed like it would just cause extra reading without any new information, and/or take up that space without much benefit.

The description also adds some complexity in terms of the manual trimming required, and it's static so if the MD file it links to changes, the link will no longer match. And alternative here would be what we kinda already discussed, which would be truncating the description maybe to one line and add an ellipsis (...) at the end?

That being said, I think my honest favorite of the options so far would be that last one:
image

I think the one with two icons gives a feeling like each line links to something different... and I kinda like including the path with the name, it very clearly feels like an internal link that won't take them too far.

@samajammin
Copy link
Contributor

I tend to agree, I think adding a description w/ every link is a bit unnecessary, especially if it's something that must be added manually. IMO I think linking to the page w/ ("More on [topic]...") is sufficient. I don't think displaying the path adds much value, given that we already clearly delineate internal vs external links w/ the arrow icon.

@wackerow
Copy link
Member Author

@samajammin Would you suggest using this option then?
image

...or just ditching the component idea entirely and leaving the regular text links as they were?

image

@samajammin
Copy link
Contributor

Either works for me, happy to differ to you & @ryancreatescopy.

@wackerow
Copy link
Member Author

That means you @ryancreatescopy ! haha (I prefer the look of the component... "buttons" always seem so much more enticing to click hah)
Just lemme know and we can wrap it up one way or the other 👍

@ryancreatescopy
Copy link
Contributor

Either works for me, happy to differ to you & @ryancreatescopy.

@samajammin @wackerow let's go with the page emoji and arrow option. I think this makes them more noticeable and would also work really well in the developer docs. To help advertise related pages, e.g. on the consensus mehanisms page. I'd imaging using DocLink component to advertise PoS and PoW pages.

Removed `Description` prop. condensed component into page-curl emoji, a title and a right arrow, clickable as a link. Intented to be reusable elsewhere on website (uses theming, and no margins cooked in by default).
Updated glossary index to remove all "description" arguments.
@github-actions github-actions bot added the documentation 📖 Change or add documentation label Nov 12, 2020
@wackerow wackerow marked this pull request as ready for review November 12, 2020 18:38
@wackerow wackerow changed the title [WIP] Internal link component [Issue #1737] Internal link component [Issue #1737] Nov 12, 2020
@wackerow
Copy link
Member Author

Merged updated dev branch and fixed conflicts.

This component now takes the format <DocLink title="" to="" /> to populate a clickable div with hover-over styling:

Light mode

image

Light mode - hover

image

Dark mode

image

Dark mode - hover

image
Responsive sizes on mobile, maintaining alignment with larger names

Mobile-size demo

image

Baked in margin is zero so this can more easily be reused elsewhere outside of Glossary. Currently this is the only implementation, and all internal glossary links have been refactored to use this.

samajammin
samajammin previously approved these changes Nov 12, 2020
Copy link
Contributor

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks again @wackerow!

src/components/DocLink.js Outdated Show resolved Hide resolved
@samajammin samajammin merged commit 2fa3289 into ethereum:dev Nov 12, 2020
@samajammin
Copy link
Contributor

@wackerow one minor issue I'm noticing - it appears the entire card is not clickable. See video:
https://share.getcloudapp.com/qGuv2NvD

Ideally the entire component would have the hover effect & link when someone mouses over it.

@wackerow wackerow deleted the w/link-component#1737 branch November 12, 2020 20:50
@wackerow
Copy link
Member Author

@samajammin Got it, good catch... I'll fix this in a separate PR 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits documentation 📖 Change or add documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants