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(gatsby-www-components): create package and add some components (#19036) #20054

Closed

Conversation

Qovaros985
Copy link
Contributor

Description

First step in works on #19036. In this PR I created a new package called gatsby-www-components and added some of the mentioned components to this package.

Questions

  1. How should this PR be tested before merging?
  2. Should this package be published in the future?
    2.1. If yes, who could I contact about this process?
    2.2. If no, how should it be used in www folder?

@Qovaros985 Qovaros985 requested a review from a team as a code owner December 10, 2019 20:11
}}
key={item}
>
<Link to={`www.youtube.com`}>{item}</Link>
Copy link
Contributor

Choose a reason for hiding this comment

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

this line is different to the current https://github.com/gatsbyjs/gatsby/blob/master/www/src/components/horizontal-nav-list.js

Suggested change
<Link to={`www.youtube.com`}>{item}</Link>
<Link to={`${slug.slice(0, -1)}#${item.toLowerCase()}`}>{item}</Link>

Copy link
Contributor

@muescha muescha Dec 11, 2019

Choose a reason for hiding this comment

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

also the Link and mediaQueries import is a little bit different from the source file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line is different to the current https://github.com/gatsbyjs/gatsby/blob/master/www/src/components/horizontal-nav-list.js

Sorry, I used that for debugging and forgot to change it back. It is fixed now.

also the Link and mediaQueries import is a little bit different from the source file

I changed it because I found copying whole gatsby-plugin-theme-ui a little bit of an overkill. I changed import for link, because in gatsby-link README.md it says not to import from there anymore and use gatsby package instead.

link: /docs/cheat-sheet/
- title: Glossary
link: /docs/glossary/
items:
Copy link
Contributor

@muescha muescha Dec 11, 2019

Choose a reason for hiding this comment

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

missing fresh content (#19965 #20065)

Suggested change
items:
items:
- title: Node.js
link: /docs/glossary/node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Qovaros985 Qovaros985 force-pushed the gatsby-www-components/add-package branch from f12878a to 8708bce Compare December 11, 2019 23:55
@vladar vladar requested a review from tesseralis January 13, 2020 16:54
Copy link
Contributor

@tesseralis tesseralis left a comment

Choose a reason for hiding this comment

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

Thanks for this work!

In addition to the inline changes requested, could you remove the components added here from www and exchange them with the components in this package? This would be a good way to test that the changes actually work.

As for publishing, I'm not sure what's the best way to do that. @fk, what was the process for gatsby-design-tokens?

@@ -0,0 +1 @@
/*.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you adding all js files to the gitignore? Was this something you added in debugging that got accidentally merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember why it is there but it seems unnecessary, so I removed it.

@@ -0,0 +1,52 @@
import React from "react"

import docsHierarchy from "../data/sidebars/doc-links.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

These files are used by more components than just this (see gatsby-node and item-list). It doesn’t make sense to move these YAML files to a "components" file when they're more like data integral to the site itself. I would leave the guide-list component out of this initial PR until we figure out what to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I removed them from PR

@fk
Copy link
Contributor

fk commented Jan 14, 2020

As for publishing, I'm not sure what's the best way to do that. @fk, what was the process for gatsby-design-tokens?

To ask @gatsbyjs/core for help :D <3 — I think @sidharthachatterjee published for me.
Sorry I can't help more. :-( 🙏

@Qovaros985
Copy link
Contributor Author

Qovaros985 commented Jan 14, 2020

In addition to the inline changes requested, could you remove the components added here from www and exchange them with the components in this package? This would be a good way to test that the changes actually work.

Ok, but my question is how should I use this package if it is not published yet. Should I use it as a local package? That is what I did for now, but it is most likely not working, as there are some peerDependencies in this package.

@Qovaros985 Qovaros985 force-pushed the gatsby-www-components/add-package branch from 8708bce to b15b82b Compare January 14, 2020 19:06
@Qovaros985 Qovaros985 requested a review from a team as a code owner January 14, 2020 19:06
@Qovaros985 Qovaros985 force-pushed the gatsby-www-components/add-package branch 2 times, most recently from bdd933e to 0abeac2 Compare January 14, 2020 19:24
@Qovaros985 Qovaros985 requested a review from a team as a code owner January 14, 2020 19:24
@Qovaros985 Qovaros985 force-pushed the gatsby-www-components/add-package branch from 0abeac2 to ac5335e Compare January 16, 2020 17:52
@Qovaros985 Qovaros985 force-pushed the gatsby-www-components/add-package branch from ac5335e to ef14f01 Compare January 16, 2020 18:00
@tesseralis
Copy link
Contributor

So it turns out that a lot of these components are already made globally available through gatsby-plugin-theme-ui. This means that we don't actually have to put them in another package -- we just need to add them to the list of exported components. I've updated the original issue (#19036) to reflect the change in plans.

As such, I'm going to close this for now to avoid confusion. @Qovaros985 thank you for your work, and would you like to take a shot of the updated issue? (It'll probably be easier than figuring out how to publish a package)

@LekoArts LekoArts closed this Feb 25, 2020
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.

5 participants