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

feat(website): SiteLink Component, replace Link with SiteLink #135

Merged
merged 5 commits into from Oct 15, 2019

Conversation

@richbachman
Copy link
Collaborator

richbachman commented Oct 14, 2019

  • SiteLink component
  • Replaced instances of Link with SiteLink
  • Fix focus and active box-shadow style so it now uses the shadowFocus token

I attempted to use Anchor as Link, but that caused conflicts with the href and to props respectively. If we used this method, we also lost the prefetching Gatsby provides with Link. This being one of the larger benefits we get get with Gatsby, and makes for a faster user experience on the site.

I decided the best path forward was to create a styled component that wrapped Link and used the token styles from Anchor.

I also found a minor bug where we're not using the shadowFocus token in Anchor and Button. Anchor is fixed, and I've created a ticket to fix Button since that has more complex styles.

@now

This comment has been minimized.

Copy link

now bot commented Oct 14, 2019

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

🔍 Inspect: https://zeit.co/twilio-dsys/paste/k03tjs2dh
🌍 Preview: https://paste-git-site-link-component.twilio-dsys.now.sh

@TheSisb

This comment has been minimized.

Copy link
Collaborator

TheSisb commented Oct 15, 2019

This is great stuff!

One of the issues that brought this up was that if you go to the index page and click "View components", then click on "Anchor", then hit your browser back button you go back to the index page. That "View Components" link is actually a Button so it doesn't use the gatsby link behavior.

I think we may also need a SiteButtonLink that uses https://www.gatsbyjs.org/docs/gatsby-link/#how-to-use-the-navigate-helper-function

@richbachman

This comment has been minimized.

Copy link
Collaborator Author

richbachman commented Oct 15, 2019

@TheSisb Right. That was in another ticket, but its actually a pretty easy fix. Use can just use our standard buttons with an onClick event like this:

<Button
    size="default"
    variant="secondary"
    onClick={() => {
        navigate('/components');
    }}
>
    View Components
</Button>

I tested it using the home -> view components -> anchor -> back button flow mentioned above. Feel free to check it out here: https://paste-68n1xdjmq.now.sh/components

Do you think will work, or do you think we need to add a SiteButtonLink link still?

@TheSisb

This comment has been minimized.

Copy link
Collaborator

TheSisb commented Oct 15, 2019

I think what you've done is fine 👍

Copy link
Collaborator

TheSisb left a comment

LGTM!

@richbachman richbachman merged commit 1295a33 into master Oct 15, 2019
6 checks passed
6 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: applitools Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: prettier Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
now Deployment has completed
Details
@richbachman richbachman deleted the site-link-component branch Oct 15, 2019
@@ -49,16 +50,34 @@ const IndexPage: React.FC<{}> = (): React.ReactElement => {
</P>
<Box marginTop="space80" marginBottom="space80">
<Box display="inline" marginRight="space40">
<Button as="a" href="/getting-started/" size="default" variant="primary">
<Button

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Oct 16, 2019

Collaborator

😟 We now have <button />s that act as links on our homepage. Is there a way we can still use the navigate API and switch out the element back to an anchor <a />?

This comment has been minimized.

Copy link
@richbachman

richbachman Oct 16, 2019

Author Collaborator

@SiTaggart We could revert back to an <a />, prevent the default href action, and then use navigate in an onClick. What do you think of that approach?

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Oct 17, 2019

Collaborator

yeah, just use the as prop, add a matching href so if I command-click it still loads the url in a new tab, and use the onClick to do client side routing for gatsby. Use :allthethings:

This comment has been minimized.

Copy link
@richbachman

richbachman Oct 21, 2019

Author Collaborator

@SiTaggart done here: #146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.