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 external doc sidebar links and replace "Changelog" with one. #1273

Merged
merged 15 commits into from
May 9, 2020

Conversation

rogermparent
Copy link
Contributor

Fixes #1272

Keep in mind that this is meant as a quick fix, and the current sidebar system
is likely to be overhauled in the future in a way that will have this
functionality, but not necessarily invoked with the same "type" syntax.

This branch allows the sidebar to have external links, and changes the
"Changelog" link at the end to be a link to the DVC GitHub.

Both the sidebar utils and component had to be changed to enable this- the
sidebar utils moreso as the "external" type bypasses previous logic entirely.
Anything without "external" set on it is treated the same, however.

The component change just treats external-typed entries a little differently by
setting target=_blank and preventing the href from having anything prepended
to it. The best solution is to move this out of the component level entirely,
but the best solution isn't the quickest and I've already mentioned big sidebar
changes are coming soon.

Keep in mind that this is meant as a quick fix, and the current sidebar system
is likely to be overhauled in the future in a way that will have the
functionality, but not necessarily invoked with the same "type" syntax.

This branch allows the sidebar to have external links, and changes the
"Changelog" link at the end to be a link to the DVC GitHub.

Both the sidebar utils and component had to be changed to enable this- the
sidebar utils moreso as the "external" type bypasses previous logic entirely.
Anything without "external" set on it is treated the same, however.

The component change just treats external-typed entries a little differently by
setting `target=_blank` and preventing the `href` from having anything prepended
to it. The best solution is to move this out of the component level entirely,
but the best solution isn't the quickest and I've already mentioned big sidebar
changes are coming soon.
@shcheklein shcheklein had a problem deploying to dvc-landing-sidebar-ext-q1ax5c May 8, 2020 22:35 Failure
My mistake, I thought `null` was the proper syntax here and I guess the linter
missed it.
@rogermparent rogermparent temporarily deployed to dvc-landing-sidebar-ext-q1ax5c May 8, 2020 22:43 Inactive
@rogermparent
Copy link
Contributor Author

I'm sure we're going to want the "Changelog" entry in right now to be labeled and/or pointing to another link. I just have what's there as an example right now.

@rogermparent rogermparent marked this pull request as draft May 8, 2020 22:43
@shcheklein shcheklein had a problem deploying to dvc-landing-sidebar-ext-zhopip May 8, 2020 23:05 Failure
I just grabbed an SVG we already had. It could use some work, but this is a
simple working implementation so it's worth pushing.
@rogermparent rogermparent had a problem deploying to dvc-landing-sidebar-ext-zhopip May 8, 2020 23:14 Failure
@rogermparent rogermparent temporarily deployed to dvc-landing-sidebar-ext-zhopip May 8, 2020 23:21 Inactive
Some parts of the application still use the sidebar.json helpers to determine
their links, and when attempting to do so on external-type links broke the
build.

This change fixes that situation, letting the `findChildWithSource` function
adapt to input that isn't internal.

It's a bit ugly, but this whole branch is a bit ugly. The sidebar overhaul
should take care of this more cleanly in the future.
@shcheklein shcheklein temporarily deployed to dvc-landing-sidebar-ext-pezxhp May 9, 2020 00:36 Inactive
I'm sorry, they look like they took some effort to write! But that's also the
reason we have to stop writing these and get rid of the old ones.
@rogermparent rogermparent temporarily deployed to dvc-landing-sidebar-ext-pezxhp May 9, 2020 00:53 Inactive
@rogermparent
Copy link
Contributor Author

I had to wrestle with the sidebar logic some more, but I think it's ready to go unless we want to change how it looks.

This PR now has:

  1. Working external links on the sidebar with little external icons
    (We can probably put a little more work in to change the color if that's important).

  2. Changelog links changed to DVC's GitHub releases everywhere (sidebar and community page)

  3. Old changelog articles removed.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Can the community page link open in a new window like the links in the User Content section? Thanks

image

https://dvc-landing-sidebar-ext-pezxhp.herokuapp.com/community

I made a new SVG based off the other one. It uses "currentColor" so it will take
on the color of its surrounding font by default, and also is fully transparent.

This new SVG is only used in the sidebar- other external link icons are still
the old hard-coded blue and white one.
@rogermparent rogermparent temporarily deployed to dvc-landing-sidebar-ext-pezxhp May 9, 2020 01:50 Inactive
@rogermparent rogermparent temporarily deployed to dvc-landing-sidebar-ext-pezxhp May 9, 2020 02:05 Inactive
This is done the same way as we already do `rel="noopener noreferrer"`.
The change applies site-wide, but only to external links that don't specify a target.

`target` can still be overridden with anything, including a blank string.
@rogermparent rogermparent temporarily deployed to dvc-landing-sidebar-ext-pezxhp May 9, 2020 02:21 Inactive
@rogermparent rogermparent temporarily deployed to dvc-landing-sidebar-ext-pezxhp May 9, 2020 02:26 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented May 9, 2020

Can the community page link open in a new window like the links in the User Content section? Thanks

Done! I actually added functionality to our Link component that makes external links default to target="_blank" when not given a target. We already do this check and give a default rel, so there isn't any downside.

@rogermparent rogermparent marked this pull request as ready for review May 9, 2020 02:30
@rogermparent rogermparent requested a review from jorgeorpinel May 9, 2020 02:32
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

just one minor request - move the asset close to the component

@rogermparent rogermparent temporarily deployed to dvc-landing-sidebar-ext-pezxhp May 9, 2020 02:45 Inactive
@rogermparent rogermparent requested a review from shcheklein May 9, 2020 02:48
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

🚀 LGTM

@shcheklein shcheklein merged commit 26130d4 into master May 9, 2020
@rogermparent rogermparent deleted the sidebar-external-links-patch branch May 13, 2020 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support external links on the sidebar (quick fix)
3 participants