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

move footer icons to CSS bg images #3655

Merged
merged 3 commits into from
May 3, 2021
Merged

Conversation

sohamsshah
Copy link
Contributor

Fixes #2900

Thank you MDN team for helping me out with this PR ❤ making OSS cakewalk for me haha ;)
Anyways, please review and let me know if any changes are needed, will be active with it.

@peterbe peterbe changed the title ↗ Fixes #2900: move footer icons to CSS bg images move footer icons to CSS bg images Apr 29, 2021
>
<TwitterIcon />
<span className="visuall-hidden">MDN on Twitter</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? visually-hidden

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let me change it to Virtually. Btw let me know if other changes are needed as well :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, as Peter mentioned, you can use the existing visually-hidden class here and the other places later in the file.

Comment on lines 77 to 79
.visuall-hidden {
display: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe you will need to add this. The utility class should already be exposed by minimalist. On the relevant HTML element you should be able to add className="visually-hidden" and it should just work.

@sohamsshah
Copy link
Contributor Author

Done with the changes 👍

@peterbe
Copy link
Contributor

peterbe commented Apr 29, 2021

@sohamsshah Can you merge in the latest main and push? (no rebase please)
Because, confusingly, the client/build/static/js/main.[hash].chunk.js file is currently larger as of this branch.

@sohamsshah
Copy link
Contributor Author

Done with the changes, Please check :)

Copy link
Contributor

@schalkneethling schalkneethling left a comment

Choose a reason for hiding this comment

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

🎉 ❤️ Approved! Thank you so much for your valued contribution @sohamsshah

@Ryuno-Ki
Copy link

Ryuno-Ki commented May 1, 2021

The base branch requires all commits to be signed. Learn more about signing commits.

Do you know how to handle this, @sohamsshah?

@sohamsshah
Copy link
Contributor Author

The base branch requires all commits to be signed. Learn more about signing commits.

Do you know how to handle this, @sohamsshah?

Interesting. Can you please brief me more about how to sign commits? Or send some resource to read about?

Thanks.

@Ryuno-Ki
Copy link

Ryuno-Ki commented May 1, 2021

I copied GitHub's link to https://docs.github.com/en/github/authenticating-to-github/about-commit-signature-verification#gpg-commit-signature-verification (without the anchor for the above comment).

Basically, it makes you create a GPG key (outlined in the above link) and upload it to GitHub.
This way, we can cryptographically verify, that you are the true author of the changes you made here.
(That's important since there were attacks in other projects like PHP. A commit was created seemingly by a maintainer, but contained malware. Since there was no verified commits process, it went unnoticed for a while).

If you make changes within GitHub's web interface, GitHub will sign your changes for you.
This will make a commit have a „Verified” badge in the UI here.

@peterbe peterbe merged commit 7a4a06b into mdn:main May 3, 2021
@peterbe
Copy link
Contributor

peterbe commented May 3, 2021

@sohamsshah Well done!! See if you can find the GitHub docs about how to set up git commit signing. It's a pain the back but you hopefully only have to do it once.

Now, we need a more challenging bug for you to solve. :)
Got anything in mind?

@sohamsshah
Copy link
Contributor Author

Thank you so much @peterbe and @schalkneethling !
Yes, I will setup the git commit signing for sure. Haha, yes sure; up for newer challenges to solve MDN ❤

I will have a look at the existing issues. Curious, if you can also suggest me some good issue, so that I can start working on it :)

@schalkneethling
Copy link
Contributor

Thank you so much @peterbe and @schalkneethling !
Yes, I will setup the git commit signing for sure. Haha, yes sure; up for newer challenges to solve MDN ❤

I will have a look at the existing issues. Curious, if you can also suggest me some good issue, so that I can start working on it :)

What technologies are you most interested in or excited to work with?

@sohamsshah
Copy link
Contributor Author

I am mostly into Frontend Development and love to explore component designs and architectures. Though, anything involving JS would excite me equally.

@peterbe
Copy link
Contributor

peterbe commented May 4, 2021

I will have a look at the existing issues. Curious, if you can also suggest me some good issue, so that I can start working on it :)

https://github.com/mdn/yari/issues?q=is%3Aopen+is%3Aissue+label%3Afrontend+no%3Aassignee is a list of front-end issues that are not already assigned.

peterbe pushed a commit to peterbe/yari that referenced this pull request Jun 1, 2021
* ↗ Fixes mdn#2900: move footer icons to CSS bg images

* replace with utility class visually-hidden
wbamberg pushed a commit to wbamberg/yari that referenced this pull request Jul 29, 2021
* upstream/main: (288 commits)
  remove tests for obsolete search endpoints (mdn#3688)
  Limit non-English locales (mdn#3661)
  move footer icons to CSS bg images (mdn#3655)
  not all http:// links can be https:// (mdn#3657)
  add the lost part and update the translation for Express (mdn#3664)
  update the translation of the zh-TW (mdn#3663)
  Add a Specifications macro and extract a special specification section to the Document (mdn#3518)
  build(deps-dev): bump & migrate use-debounce to 6.0.0 (mdn#3684)
  build(deps-dev): bump puppeteer from 9.0.0 to 9.1.0 (mdn#3686)
  build(deps-dev): bump & migrate husky to 6.0.0 (mdn#3681)
  build(deps-dev): bump jest-environment-jsdom-sixteen from 1.0.3 to 2.0.0 (mdn#3615)
  More concurrent open npm PRs (mdn#3685)
  build(deps-dev): bump black from 21.4b0 to 21.4b2 in /deployer (mdn#3668)
  build(deps-dev): bump black in /testing/integration (mdn#3666)
  build(deps-dev): bump @babel/core from 7.13.16 to 7.14.0 (mdn#3683)
  build(deps-dev): bump jest-puppeteer from 5.0.2 to 5.0.3 (mdn#3680)
  build(deps): bump @mdn/browser-compat-data from 3.3.0 to 3.3.2 (mdn#3679)
  build(deps-dev): bump sass from 1.32.11 to 1.32.12 (mdn#3678)
  build(deps-dev): bump eslint-plugin-jest from 24.3.5 to 24.3.6 (mdn#3677)
  build(deps): bump http-proxy-middleware from 1.3.0 to 1.3.1 (mdn#3675)
  ...
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.

Move icons in footer to CSS background images
4 participants