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: css magazine #43507

Merged
merged 27 commits into from Oct 28, 2021
Merged

feat: css magazine #43507

merged 27 commits into from Oct 28, 2021

Conversation

naomi-lgbt
Copy link
Member

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the main branch of freeCodeCamp.
  • I have tested these changes either locally on my machine, or GitPod.

Closes #XXXXX

Here's the steps. Tests coming soon:tm:

@gitpod-io
Copy link

gitpod-io bot commented Sep 20, 2021

@github-actions github-actions bot added platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. labels Sep 20, 2021
@naomi-lgbt naomi-lgbt marked this pull request as ready for review September 22, 2021 21:50
@naomi-lgbt naomi-lgbt requested a review from a team as a code owner September 22, 2021 21:50
@ShaunSHamilton ShaunSHamilton self-assigned this Sep 22, 2021
Copy link
Member

@gikf gikf left a comment

Choose a reason for hiding this comment

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

Up to part 40 only thing that was visible in the preview, without scrolling it, was logo. I'd consider moving part with resizing it to some earlier part.

Small findings from completing project below.

@gikf gikf added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Sep 28, 2021
@raisedadead raisedadead added status: merge conflict To be applied to PR's that have a merge conflict and need updating and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Oct 1, 2021
Copy link
Contributor

@scissorsneedfoodtoo scissorsneedfoodtoo left a comment

Choose a reason for hiding this comment

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

Hey @nhcarrigan, sorry for just a partial review. Everything is looking great so far! Should be a fun project. Lots of great tests and hints throughout, too.

I noticed some things going through and left them as review comments.

I'll finish going through the rest of project ASAP.

Copy link
Contributor

@scissorsneedfoodtoo scissorsneedfoodtoo left a comment

Choose a reason for hiding this comment

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

Again, this is looking great @nhcarrigan. I had a ton of fun going through all these steps.

Feel free to disregard any of my review comments, or to tag me for more discussion. They're all just things I noticed while going through the project.

@naomi-lgbt naomi-lgbt added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: merge conflict To be applied to PR's that have a merge conflict and need updating labels Oct 6, 2021
Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

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

Some necessary suggestions. Some, because I think it would be more usable.

I have stopped reviewing at part-013, because GitHub is acting up. More suggestions are likely to come.

@ghost

This comment has been minimized.

Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

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

GitHub is making this more difficult than it normally is. So, apologies for the staggered reviews.


After going through this, I am leaning towards the suggestion of:

  • Instructions: Add a p element with at least 10 characters...
  • Then, the next lesson has all this boilerplate content

Overall, really satisfying to finish, but, at the same time, I was waiting, and waiting, and waiting for the .hero-title to be fixed on the preview so it did not cover the img:
image

That never happened, and it does look like a novice mistake - overlaying an image with text like that. I think we should consider fixing it by either:
a) Centering the .hero-title
b) Not setting it to position: absolute, and decreasing the font-size to something reasonable in the scale. e.g. maximum of 4rem. 3.6rem would actually be next in the scale

@naomi-lgbt
Copy link
Member Author

Overall, really satisfying to finish, but, at the same time, I was waiting, and waiting, and waiting for the .hero-title to be fixed on the preview so it did not cover the img:

This could very well be a result of changing the content of this project, too. I'll give this a play and see what I come up with.

@naomi-lgbt
Copy link
Member Author

naomi-lgbt commented Oct 14, 2021

Merge @ShaunSHamilton's PR, get conflicts on my own. 😠

Will apply review + resolve conflicts in the morning.

Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

Found a few bugs and typos - and some firefix issues

naomi-lgbt and others added 23 commits October 27, 2021 12:46
Co-authored-by: Krzysztof <60067306+gikf@users.noreply.github.com>
Co-authored-by: Krzysztof <60067306+gikf@users.noreply.github.com>
Co-authored-by: Krzysztof <60067306+gikf@users.noreply.github.com>
Co-authored-by: Sem Bauke <46919888+Sembauke@users.noreply.github.com>
Co-authored-by: Sem Bauke <46919888+Sembauke@users.noreply.github.com>
Co-authored-by: Kristofer Koishigawa <scissorsneedfoodtoo@gmail.com>
Co-authored-by: Kristofer Koishigawa <scissorsneedfoodtoo@gmail.com>
Co-authored-by: Shaun Hamilton <shauhami020@gmail.com>
Co-authored-by: Shaun Hamilton <shauhami020@gmail.com>
Co-authored-by: Tom <20648924+moT01@users.noreply.github.com>
Co-authored-by: Tom <20648924+moT01@users.noreply.github.com>
Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

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

This LGTM 👍

Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@moT01 moT01 merged commit cb5244b into freeCodeCamp:main Oct 28, 2021
@naomi-lgbt naomi-lgbt deleted the feat/magazine branch October 28, 2021 20:28
@naomi-lgbt
Copy link
Member Author

time for some rebases 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants