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

Add Link component #209

Merged
merged 2 commits into from
Jul 14, 2020
Merged

Add Link component #209

merged 2 commits into from
Jul 14, 2020

Conversation

Joozty
Copy link
Member

@Joozty Joozty commented Jul 9, 2020

Closes #205

@vercel
Copy link

vercel bot commented Jul 9, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oacore/design/7kuh43me2
✅ Preview: https://design-git-link.oacore.vercel.app

@Joozty
Copy link
Member Author

Joozty commented Jul 13, 2020

@viktor-yakubiv Don't you know by any chance the name of the external icon? It's like finding a needle in the haystack.

@vercel vercel bot requested a deployment to Preview July 13, 2020 15:51 Abandoned
@vercel vercel bot requested a deployment to Preview July 13, 2020 15:56 Abandoned
@viktor-yakubiv
Copy link
Member

If I remember, open-in-new

@viktor-yakubiv
Copy link
Member

If user click on Link in menu they are redirected to the button link instead because it has the same ID.

The backside of the fancy markdown 😄

@viktor-yakubiv
Copy link
Member

I am looking into it. I will send an update, I am exploring more cases. However, I like the overlall result. Thanks a lot for doing this 👍

@viktor-yakubiv
Copy link
Member

I found the icon looks good in Safari but in Chrome and Firefox it does not look very good in text. Feel free to check it now but I will apply some changes.

By the way, I do not recommend to change the default display for the text elements, it may be unexpected for the user.

@viktor-yakubiv
Copy link
Member

There is a problem that I see can only be solved with web-fonts at the moment (without JS processing). I wish I am wrong and there is an option with the background + mask bit obviously inline-block behaves like an image too.

Open in new icon falls to the next line

The icon should break with the last work within the link alway. white-space does not help us because it will make the whole link unbreakable.

@viktor-yakubiv
Copy link
Member

I added a simple solution but we should investigate this breaking rules more because it will become a headache when goes to production.

Joozty and others added 2 commits July 14, 2020 13:19
Adds a wrapper around the HTML Anchor Element (<a>)
to simplify working with the external links. The component
fixes the recommendation to pass `rel="noopener noreferrer"`
on its level.

Includes `open-in-new` icon into the design.config.

Co-authored-by: Viktor Yakubiv <viktor@yakubiv.com>
If the users click on Link in the menu they were redirected
to the Button Link subsection instead because it has the same
autogenerated ID.

To avoid such problems we should try to keep all headings
and subheading different within all the files. Despite it's better
to find another solution.
@viktor-yakubiv
Copy link
Member

@Joozty sorry I put my hands here maybe too much. I tried to explain my concerns as much as possible and even created an issue after this (#216). I wonder if you have any comments but if you don't feel free to rebase and merge 😉

Thanks a lot for looking into this. It was interesting investigation, I hope for both of us 🙂

@Joozty
Copy link
Member Author

Joozty commented Jul 14, 2020

The icon should break with the last work within the link alway. white-space does not help us because it will make the whole link unbreakable.

You would need another wrapper I guess.

<a>Link with the last <span>word<svg>icon here</svg></span></a>

Anyway thanks for looking into this. As I always say I don't see any difference whether the element is inline flex or just inline. 😄

@Joozty Joozty merged commit 9d68c46 into master Jul 14, 2020
Joozty added a commit that referenced this pull request Jul 14, 2020
Adds a wrapper around the HTML Anchor Element (<a>)
to simplify working with the external links. The component
fixes the recommendation to pass `rel="noopener noreferrer"`
on its level.

Includes `open-in-new` icon into the design.config.

Co-authored-by: Viktor Yakubiv <viktor@yakubiv.com>
@Joozty Joozty deleted the link branch July 14, 2020 10:53
@viktor-yakubiv
Copy link
Member

This is exactly the solution I was thinking about but it needs language processing on JS what I would like to avoid. I would rather have this defect than handle it in such way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚛️ components New components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop a Link component
2 participants