Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

feat: AuthorsList Component with Github Profile Photos#215

Merged
ahmadawais merged 15 commits intomasterfrom
ahmadawais/contributors
Apr 2, 2019
Merged

feat: AuthorsList Component with Github Profile Photos#215
ahmadawais merged 15 commits intomasterfrom
ahmadawais/contributors

Conversation

@ahmadawais
Copy link
Copy Markdown
Member

@ahmadawais ahmadawais commented Mar 31, 2019

Built a new AuthorsList component and refactored the old AuthorsLink component into Author component. Now the contributors list at the bottom of an article has GitHub profile images with brand color borders. 🎉

FIXES: #209

Demo:

This is what it looks like with a couple of contributors.

demo

@ahmadawais ahmadawais self-assigned this Mar 31, 2019
@ahmadawais ahmadawais added the enhancement New feature or request label Mar 31, 2019
LaRuaNa
LaRuaNa reviewed Mar 31, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@LaRuaNa LaRuaNa left a comment

Choose a reason for hiding this comment

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

Cool, LGTM. Just one thing I mentioned above :]

@nodejs nodejs deleted a comment from github-actions bot Mar 31, 2019
@ahmadawais ahmadawais requested a review from LaRuaNa March 31, 2019 10:05
@nodejs nodejs deleted a comment from github-actions bot Mar 31, 2019
@ahmadawais
Copy link
Copy Markdown
Member Author

ahmadawais commented Mar 31, 2019

/gcbrun

Fixed that yarn lock file.

cc: @LaRuaNa would need another review now 👍

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 31, 2019

Preview at: https://storage.googleapis.com/staging.nodejs.dev/87756f3/index.html

@LaRuaNa
Copy link
Copy Markdown
Contributor

LaRuaNa commented Mar 31, 2019

@ahmadawais Emotion is already available see #172 Can you please do it in the same way as in the PR so we'd have the styles also in snapshots. And revert another changes? Everything else still LGTM 👍

@ahmadawais
Copy link
Copy Markdown
Member Author

ahmadawais commented Mar 31, 2019

@LaRuaNa I really like the styled components approach to this. Can that not work with snapshots? Are we testing css there as well?🤔

@LaRuaNa
Copy link
Copy Markdown
Contributor

LaRuaNa commented Mar 31, 2019

Actually I'm not fussed styled or inline we just started making snapshots with inline CSS and I'd like to keep it consistent. I'm not familiar enough with both methods but if you say styled way is better maybe write a RFC so we can talk about it in the next meeting. But by then I think we better keep it consistent :]

sagirk
sagirk suggested changes Mar 31, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@sagirk sagirk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Fantastic work @ahmadawais! ❤️

I noticed that the default exports in the new component files are not named. This is probably not a good idea — unnamed functions show up as anonymous in a stack trace. This is not very helpful when debugging.

Request you to revert to the established pattern of naming the exports.

Also, this may necessitate renaming the styled components that conflict with the exported component name. This is where I feel using the css approach is more elegant over styled (not sure if it adds any weight, but noticed that most of the examples in the official emotion docs use css).

@BeniCheni
Copy link
Copy Markdown
Contributor

BeniCheni commented Apr 2, 2019

From @LaRuaNa:

Actually I'm not fussed styled or inline we just started making snapshots with inline CSS and I'd like to keep it consistent ...

From @sagirk:

Or better yet, switch to the css way of doing it as previously done in #172 ...

Per these previous comments about inline CSS v.s. styled "components", (also observed that there is also code that uses class names), I do agree that it'd be great, if we can agree on one or the other pattern for styles.

BeniCheni
BeniCheni reviewed Apr 2, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@BeniCheni BeniCheni left a comment

Choose a reason for hiding this comment

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

LGTM with a "tiny" code question. Tested and looked good in staging. Thanks for doing this! ✌️

@ahmadawais ahmadawais requested a review from sagirk April 2, 2019 14:12
@ahmadawais
Copy link
Copy Markdown
Member Author

ahmadawais commented Apr 2, 2019

@LaRuaNa @sagirk @BeniCheni all done. Let's merge this tonight.

/gcbrun

@ahmadawais
Copy link
Copy Markdown
Member Author

ahmadawais commented Apr 2, 2019

From @BeniCheni

Per these previous comments about inline CSS v.s. styled "components", (also observed that there is also code that uses class names), I do agree that it'd be great, if we can agree on one or the other pattern for styles.

True that. I am a big fan of the styled components model. Makes things super minimal and fun to read. Don't much like to name things like consts for the css attrib. I'd vote for the styled attribute here. 👍

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2019

Preview at: https://storage.googleapis.com/staging.nodejs.dev/f5bb3a7/index.html

sagirk
sagirk reviewed Apr 2, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@sagirk sagirk left a comment

Choose a reason for hiding this comment

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

Great job! Looks fantastic, @ahmadawais! 💯

A minor nit below. Also, considering the authors-list.tsx component is introduced by this PR, do you mind adding tests for it?

sagirk
sagirk approved these changes Apr 2, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@sagirk sagirk left a comment

Choose a reason for hiding this comment

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

Oh, and BTW, none of the above is a blocker!

Feel free to address them on a follow-up PR, if you'd prefer.

amiller-gh
amiller-gh reviewed Apr 2, 2019
View reviewed changes
amiller-gh
amiller-gh previously requested changes Apr 2, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@amiller-gh amiller-gh left a comment

Choose a reason for hiding this comment

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

One blocking semantic HTML comment and then LGTM! Love that we have this, great job 🎉

@ahmadawais
Copy link
Copy Markdown
Member Author

ahmadawais commented Apr 2, 2019

@amiller-gh @sagirk awesome! Thanks for the reviews folks. Will need one more review to merge. I also just added the contributors for the first doc which are no in sync with GitHub contributors. Will handle the rest in a separate PR.

For now let's merge this one and work on improving things in separate PRs.

Looking forward, peace! ✌️

/gcbrun

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2019

Preview at: https://storage.googleapis.com/staging.nodejs.dev/0fc68d6/index.html

LaRuaNa
LaRuaNa approved these changes Apr 2, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@LaRuaNa LaRuaNa left a comment

Choose a reason for hiding this comment

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

lovely :] good job 👍

@ahmadawais ahmadawais dismissed amiller-gh’s stale review April 2, 2019 16:20

Discussed with Adam and we're good to go in a separate PR.

@ahmadawais ahmadawais merged commit 7038cd7 into master Apr 2, 2019
@sagirk sagirk deleted the ahmadawais/contributors branch April 3, 2019 02:49
@sagirk sagirk mentioned this pull request Apr 3, 2019
Merged
@ahmadawais ahmadawais restored the ahmadawais/contributors branch April 3, 2019 10:25
ahmadawais pushed a commit that referenced this pull request Apr 3, 2019
@sagirk
fix: Add tests and make markup semantic (outstanding nits from #215) (#…
1ee4beb 
…217)

<!--
Please read the [Code of Conduct](https://github.com/nodejs/nodejs.dev/blob/master/CODE_OF_CONDUCT.md) and the [Contributing Guidelines](https://github.com/nodejs/nodejs.dev/blob/master/CONTRIBUTING.md) before opening a pull request.
-->

## Description

<!-- Write a brief description of the changes introduced by this PR -->

This is a follow-up PR to #215. It addresses outstanding nits.

## Related Issues

<!--
  Link to the issue that is fixed by this PR (if there is one)
  e.g. Fixes #1234, Addresses #1234, Related to #1234, etc.
-->
Ref: https://github.com/nodejs/nodejs.dev/issues/209
kevindavies8 added a commit to kevindavies8/nodejs.dev-for-full-stack-developer that referenced this pull request Aug 24, 2022
@kevindavies8
fix: Add tests and make markup semantic (outstanding nits from #215) (#…
6b46ce7 
…217)

<!--
Please read the [Code of Conduct](https://github.com/nodejs/nodejs.dev/blob/master/CODE_OF_CONDUCT.md) and the [Contributing Guidelines](https://github.com/nodejs/nodejs.dev/blob/master/CONTRIBUTING.md) before opening a pull request.
-->

## Description

<!-- Write a brief description of the changes introduced by this PR -->

This is a follow-up PR to nodejs/nodejs.dev#215. It addresses outstanding nits.

## Related Issues

<!--
  Link to the issue that is fixed by this PR (if there is one)
  e.g. Fixes #1234, Addresses #1234, Related to #1234, etc.
-->
Ref: https://github.com/nodejs/nodejs.dev/issues/209
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@BeniCheni BeniCheni BeniCheni left review comments

@LaRuaNa LaRuaNa LaRuaNa approved these changes

@sagirk sagirk sagirk approved these changes

@amiller-gh amiller-gh amiller-gh left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ahmadawais ahmadawais

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ahmadawais @LaRuaNa @BeniCheni @sagirk @amiller-gh