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

added normalize.css package as reset css #1071

Open
wants to merge 2 commits into
base: features/ux
Choose a base branch
from

Conversation

akiskips
Copy link
Contributor

@akiskips akiskips commented Oct 20, 2024

πŸ› οΈ Description

Added normalize.css package from https://github.com/necolas/normalize.css/
Added import at main.tsx

Fixes #

πŸ“· Screenshots

πŸ“‹ Checklist

πŸ”¬ How did you test this change?

  • 🀏 Lint tests
  • 🀞 PS -WhatIf / az validate
  • πŸ‘ Manually deployed + verified
  • πŸ’ͺ Unit tests
  • πŸ™Œ Integration tests

πŸ™‹β€β™€οΈ Do any of the following that apply?

  • 🚨 This is a breaking change.
  • 🀏 The change is less than 20 lines of code.

πŸ“‘ Did you update docs/changelog.md?

  • βœ… Updated changelog (required for dev PRs)
  • ➑️ Will add log in a future PR (feature branch PRs only)
  • ❎ Log not needed (small/internal change)

πŸ“– Did you update documentation?

  • βœ… Public docs in docs (required for dev)
  • βœ… Internal dev docs in src (required for dev)
  • ➑️ Will add docs in a future PR (feature branch PRs only)
  • ❎ Docs not needed (small/internal change)

@akiskips
Copy link
Contributor Author

@Ivanmtta Not sure if it would be better to add an @import to index.css instead

@akiskips akiskips added the Micro PR πŸ”¬ Very small PR that should be especially easy for newcomers label Oct 20, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Micro PR πŸ”¬ Very small PR that should be especially easy for newcomers label Oct 20, 2024
@@ -12,6 +12,7 @@
},
"dependencies": {
"eslint-plugin-react": "^7.37.1",
"normalize.css": "^8.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is necessary. Cold we just add this to the main css file?

* {
  padding: 0;
  margin: 0;
  box-sizing: border-box;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure we can do that . I removed the package and added the reset rule in the start of index.css
My idea was to use the normalize.css that seems to add a variaty of reset rules for different browsers. But it adds to complexity and probably we dont need it atm:

https://github.com/necolas/normalize.css/blob/master/normalize.css

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Ivanmtta Are you suggesting that those are the only 3 rules we need? It's been a while since I've looked at resets, but I would expect a lot more to be needed than just these. Colors, sizes,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ivan and I discussed this. We agreed to revert back to normalize.css out of the box.

@akiskips You can resolve this comment when you've done that. Thanks!

@flanakin flanakin added this to the Web app milestone Oct 31, 2024
@flanakin flanakin added Tool: Web app Web app to learn about and manage toolkit resources. Needs: Attention πŸ‘‹ Issue or PR needs to be reviewed by the author or it will be closed due to no activity and removed Needs: Review πŸ‘€ PR that is ready to be reviewed labels Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention πŸ‘‹ Issue or PR needs to be reviewed by the author or it will be closed due to no activity Tool: Web app Web app to learn about and manage toolkit resources.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants