Upgrades ProductCard component (adds subjects / GitHub info) [Closes #1782, Closes #1783] #1808

wackerow · 2020-11-13T07:56:02Z

Description

Adds pill-like component that pulls GitHub repo star count for projects listed on the site.

Only shows if gitAccount and gitRepo names are passed. Added these props to <ProductCard> components, and updated the data fed to them for sandboxes and frameworks.

Still need to update all of the /eth2/get-involved listings. --Update: done

Related Issue #1782

conditional upon receiving `gitAccount` and `gitRepo` as props

local-environments

/eth2/get-involved/

ryancreatescopy · 2020-11-13T16:32:39Z

Should we add the GitHub icon to make it clearer we're pulling in github data?

wackerow · 2020-11-13T16:56:48Z

@ryancreatescopy Any thoughts on where to put it? In the component? A separate indicator for each card? One larger indicator for these whole sections? My thought was squeezing more glyphs into a component like this would make it overly complicated (especially if you have both that and the star) and if someone doesn't know what the stars are, they probably don't care about them.

Just some thoughts, let me know what you think. Keep in mind I'm gonna work on a component with a horizontal/wrapping 'pill' list for each programming language... still trying to find the best way for it to all fit together nicely.

samajammin · 2020-11-13T17:33:45Z

This is amazing!

Re: the GH icon / star layout, a couple options:

Keep as-is visually but have the component be a link to the GH repo, which would make it obvious where the data is coming from. Only potential issue I see with this is a link nested within a link, which is not technically valid HTML. Perhaps we add a button to the cards which act as the link to the project.
Display the GH icon with the word "stars", like this common widget:

https://ghbtns.com/

Re: this approach to querying data:
Overall I think this is a valid v1 solution. One thing to consider is that this creates a network request on page load for every project we display, which is far from optimal. I think the ideal solution would be to query these project stars via GraphQL at build time (we already have the GH API available). In this case, the data would already be in the static HTML the user loads so we could avoid the delay & added load of all these network requests. The downside with this approach is that stars wouldn't dynamically update but given how often we deploy the site, I don't see this being a concern.

wackerow · 2020-11-13T17:50:49Z

@samajammin Agree with everything, and you've iterated some of the issues I came across in v1. I originally made this a link until I realized it was already nested (at least when used in a ProductCard).
Had some small issues using the Icons component in the first draft because of the baked in hover-over coloring, but if we move this out of a nest link, it would no longer be an issue.... just not sure where to put it exactly, since it seems to belong right inside these product cards.
I'll peak at the ghbtns.com page and mull it over, but if anyone has any thoughts, all ears

Re: Querying
Good call, and I agree that at the rate this site gets rebuilt it shouldn't be an issue (stars don't add up that fast). I had mimicked the approach taken in the Roadmaps.js component, but that seems to just be one API call, not one call per item listed like this PR. Will look at this =)

wackerow · 2020-11-14T00:55:23Z

@samajammin @ryancreatescopy Let me know what you think of this approach. The whole bar is clickable to go to github repo.

Note, links work but there is a nesting issue still which I will address (which will also address the small issue of the background highlighting going away when hovering over the project image). Also, this has not been refactored to use graphql for api calls, and there may be issues with rate limiting for-the-moment.

/developers/learning-tools/

until API issues fixed

removed redundant font-size style

added DEV boolean to test layout with this on top vs bottom. removed unneeded styling. increased padding by 50% and font size of star count from `xs` to `s`.

turquoise, yellow, mint, pink were all defined with hex strings lacking the leading "#", this corrects it

ryancreatescopy · 2020-11-18T11:32:23Z

If we don't show the GitHub stars/button, we should avoid having so much margin above the title:

wackerow · 2020-11-18T17:20:27Z

wackerow · 2020-11-18T17:29:54Z

One last note: I know it was commented about the use of 'web3' as a subject (ie. Consensys bootcamp). This can obviously be changed, but my reasoning for keeping it in at the moment is because, yes, this is truly javascript or typescript, but if someone knows javascript and they are looking to learn about Ethereum, they may not be interested in just seeing "JavaScript" since it's not obvious by that label alone that it's something new to them.
I haven't taken this Concensys course, but my assumption is they are not teaching vanilla javascript, but instead are teaching web3 which utilizes JS/TS.

Options:
1- Switch it to JavaScript and/or TypeScript
2- Perhaps change to something like web3 / JS
3- Leave as web3 subject matter and adjust with future PR if needed

reverts `display` back to inline for SubjectContainer to allow native wrapping of tags if list is longer, and removes negative margin-top on pill (adjusted container margin) and adds margin-bottom in case of multiple rows

eliminates need for hard-coding in a color for each subject tag, improving flexibility to add new subjects, and no subjects default to gray. Offset can be used to adjust theme coloring.

samajammin

This looks really solid! Left a handful of comments, then I think this is ready to merge. Awesome work.

I think there's still some polishing we may want to do re: the programming language tags but I think we wait to tweak that later if needed in a separate PR.

@wackerow just a note for future PRs: whenever possible let's try to isolate changes into individual PRs. I can understand including the programming language tags in this PR since those are also fed from the GitHub API but I would've liked to see that separated. I think keeping this PR focused solely on the GitHub stars widget would have keep the conversation more focused & would have allowed this contribution to get merged faster.

Thanks again!

samajammin · 2020-11-20T20:21:31Z

src/components/GitStars.js

+import Link from "./Link"
+
+const Container = styled(Link)`
+  margin-right: -0.5rem;


Generally I'm not a fan of negative margin - why is this needed here? If we want these components to be aligned further right, I suggest we reduct the padding of the card content.

samajammin · 2020-11-20T20:23:46Z

src/components/GitStars.js

+
+const GlyphPill = styled(Pill)`
+  display: flex;
+  flex-direection: row;


Typo here, but you can just remove this line (flex-direction defaults to row)

samajammin · 2020-11-20T20:28:58Z

src/pages-conditional/developers/local-environment.js

    openzeppelin: file(relativePath: { eq: "devtools/openzeppelin.png" }) {
      ...devtoolImage
    }
+    openzeppelinGitHub: github {
+      repository(owner: "OpenZeppelin", name: "openzeppelin-contracts") {


Suggested change

repository(owner: "OpenZeppelin", name: "openzeppelin-contracts") {

repository(owner: "OpenZeppelin", name: "openzeppelin-sdk") {

Wrong repo 😄

src/pages-conditional/developers/local-environment.js

samajammin · 2020-11-20T20:31:52Z

src/pages-conditional/developers/local-environment.js

@@ -274,6 +274,7 @@ const frameworksList = [
    name: "Waffle",
    description:
      "The most advanced testing lib for smart contracts. Use alone or with Scafold-eth or Hardhat.",
+    gitHubUrl: "https://github.com/EthWorks/waffle",


Since we're already using the GitHub API to pull in star counts, we could also use the API to pull in GitHub URL. See example code in my comment below.

src/pages-conditional/developers/local-environment.js

fixed warning about {"1rem"} passed to <Icon />, changed to "16px"

removes left-over unused query fragmet

wackerow · 2020-11-24T23:26:12Z

Sorry for delay here, after rebasing the sizing broke. Found what had changed and cleaned up an unnecessary nested div that was causing issues, it now lays out the way it had been, with slight tweaks to align tags after talking with @ryancreatescopy

src/components/ProductCard.js

samajammin

This is great! Thanks for your persistence on this 💪

wackerow · 2020-11-30T22:07:23Z

Hooray! haha no problem =)

wackerow added 6 commits November 12, 2020 20:03

github stars pill draft

22e46e9

refactored to use emoji star

03067f5

added to ProductCard component

72eb9bf

conditional upon receiving `gitAccount` and `gitRepo` as props

added git account/repo info to sandboxes

29f5273

added git account/repo info to frameworks

2a0fe5d

local-environments

changed test color to .text

a5165f2

github-actions bot added the Status: Review Needed label Nov 13, 2020

Merge branch 'dev' into w/gitstars#1782

ade42c4

wackerow marked this pull request as draft November 13, 2020 07:57

added git account/repo info to clients

0e1f424

/eth2/get-involved/

wackerow added 3 commits November 13, 2020 12:16

Merge branch 'dev' into w/gitstars#1782

7bb5bb5

refactored and combined with language pills

2ece519

Merge remote-tracking branch 'upstream/dev' into w/gitstars#1782

4089d8c

github-actions bot added the documentation 📖 Change or add documentation label Nov 14, 2020

wackerow added 11 commits November 13, 2020 17:03

(reverted github token)

e530659

fixes nested anchor tags

b18556f

added git account/repo info to games

01bd988

/developers/learning-tools/

brightened language colors slightly

dbc667f

added flex: 1 to fix height issues

ab26c0f

switches to static dummy data for dev

d5a7610

until API issues fixed

adds commas to stringified star count

ea9fd7b

used themed font size

c592965

removed redundant font-size style

slight adjustments, clean-up

5f16fda

added DEV boolean to test layout with this on top vs bottom. removed unneeded styling. increased padding by 50% and font size of star count from `xs` to `s`.

refactors language colors

23edcb7

fixes theme colors bug (missing #)

b17270a

turquoise, yellow, mint, pink were all defined with hex strings lacking the leading "#", this corrects it

alignment adjustments

9830129

wackerow added 2 commits November 18, 2020 08:57

title margin conditional on git button presense

00e2afa

git icon highlights when hovering on containing pill

8066819

wackerow changed the title ~~Adds GitHub bar to ProductCard component [Closes #1782, Closes #1783]~~ Upgrades ProductCard component (adds subjects / GitHub info) [Closes #1782, Closes #1783] Nov 18, 2020

wackerow added 3 commits November 18, 2020 14:11

allows subject tags to extend to multiple rows for longer lists

8b9a07b

reverts `display` back to inline for SubjectContainer to allow native wrapping of tags if list is longer, and removes negative margin-top on pill (adjusted container margin) and adds margin-bottom in case of multiple rows

generate color from tag name

a9b125c

eliminates need for hard-coding in a color for each subject tag, improving flexibility to add new subjects, and no subjects default to gray. Offset can be used to adjust theme coloring.

reverted hueHash

d87a7b8

samajammin requested changes Nov 20, 2020

View reviewed changes

wackerow added 8 commits November 20, 2020 13:07

negative margin/typo correction

b689de1

query cleanup, gitHubUrl passing removed, fixed OZ repo

f344560

fixed warning about {"1rem"} passed to <Icon />, changed to "16px"

reverts back to only top result for eth2 clients

31bbf45

removes left-over unused query fragmet

Merge remote-tracking branch 'upstream/dev' into w/gitstars#1782

058281e

merge conflict correction

e43f3d5

Merge remote-tracking branch 'upstream/dev' into w/gitstars#1782

c277587

fixed formatting after CardGrid implement

7dbd3b0

keeps tags aligned, and top of content aligned

03102d7

wackerow requested a review from samajammin November 24, 2020 23:26

wackerow and others added 2 commits November 25, 2020 12:31

Merge remote-tracking branch 'upstream/dev' into w/gitstars#1782

c3ad99b

Merge branch 'dev' into w/gitstars#1782

863dabd

samajammin reviewed Nov 30, 2020

View reviewed changes

src/components/ProductCard.js Outdated Show resolved Hide resolved

Update content padding

e48c9b5

samajammin approved these changes Nov 30, 2020

View reviewed changes

samajammin merged commit 9b22fbb into ethereum:dev Nov 30, 2020

wackerow deleted the w/gitstars#1782 branch November 30, 2020 22:07

samajammin mentioned this pull request Dec 1, 2020

Deploy v2.12.3 #2021

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrades ProductCard component (adds subjects / GitHub info) [Closes #1782, Closes #1783] #1808

Upgrades ProductCard component (adds subjects / GitHub info) [Closes #1782, Closes #1783] #1808

wackerow commented Nov 13, 2020 •

edited

ryancreatescopy commented Nov 13, 2020

wackerow commented Nov 13, 2020

samajammin commented Nov 13, 2020

wackerow commented Nov 13, 2020

wackerow commented Nov 14, 2020

ryancreatescopy commented Nov 18, 2020

wackerow commented Nov 18, 2020

wackerow commented Nov 18, 2020

samajammin left a comment

samajammin Nov 20, 2020

samajammin Nov 20, 2020

samajammin Nov 20, 2020

samajammin Nov 20, 2020

samajammin Nov 20, 2020

wackerow commented Nov 24, 2020

samajammin left a comment

wackerow commented Nov 30, 2020

	repository(owner: "OpenZeppelin", name: "openzeppelin-contracts") {
	repository(owner: "OpenZeppelin", name: "openzeppelin-sdk") {

Upgrades ProductCard component (adds subjects / GitHub info) [Closes #1782, Closes #1783] #1808

Upgrades ProductCard component (adds subjects / GitHub info) [Closes #1782, Closes #1783] #1808

Conversation

wackerow commented Nov 13, 2020 • edited

Description

Related Issue #1782

ryancreatescopy commented Nov 13, 2020

wackerow commented Nov 13, 2020

samajammin commented Nov 13, 2020

wackerow commented Nov 13, 2020

wackerow commented Nov 14, 2020

ryancreatescopy commented Nov 18, 2020

wackerow commented Nov 18, 2020

wackerow commented Nov 18, 2020

samajammin left a comment

Choose a reason for hiding this comment

samajammin Nov 20, 2020

Choose a reason for hiding this comment

samajammin Nov 20, 2020

Choose a reason for hiding this comment

samajammin Nov 20, 2020

Choose a reason for hiding this comment

samajammin Nov 20, 2020

Choose a reason for hiding this comment

samajammin Nov 20, 2020

Choose a reason for hiding this comment

wackerow commented Nov 24, 2020

samajammin left a comment

Choose a reason for hiding this comment

wackerow commented Nov 30, 2020

wackerow commented Nov 13, 2020 •

edited