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: adding styles guidance #6

Merged
merged 14 commits into from
May 1, 2024

Conversation

lholmquist
Copy link
Member

No description provided.

@lholmquist lholmquist marked this pull request as ready for review February 6, 2024 17:05

* rem - preferred for accesibilty and for fonts
* borders - these could be in **px**
* div tags - should be more percentage based
Copy link
Member

Choose a reason for hiding this comment

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

Disagree with this as a general rule, especially without any explanation/guidance

Copy link
Member Author

Choose a reason for hiding this comment

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

After watching the video of the meeting, there wasn't really any explanation, so i think we should talk through this and the other comment in the next meeting

The team thought that it was important to stick with one for a consistent UI and consistecy across the application. Some helpful recommendations that came out of our discussion where:

* rem - preferred for accesibilty and for fonts
* borders - these could be in **px**
Copy link
Member

Choose a reason for hiding this comment

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

why?


#### Vendor prefixes

Browser vendors would add prefixes to non-standard/new css properties for developers to experiment with. The team recommends the use of the [autoprefixer](https://www.npmjs.com/package/autoprefixer) library, which is an industry standard. While this might important when supporting older code, it is recommended to look at if and why you might need a prefix when creating new applicaitons.
Copy link
Member

Choose a reason for hiding this comment

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

should mention this is a postcss plugin and requires postcss, and maybe talk about postcss itself

@ctcpip
Copy link
Member

ctcpip commented Feb 26, 2024

consider adding content about css methodologies, e.g. BEM

docs/development/styles.md Outdated Show resolved Hide resolved
docs/development/styles.md Outdated Show resolved Hide resolved
docs/development/styles.md Outdated Show resolved Hide resolved
docs/development/styles.md Outdated Show resolved Hide resolved
@adigidh
Copy link
Contributor

adigidh commented Apr 17, 2024

Not wanting this to be added in on the docs here, but wanted to start a discussion on whether it makes sense to add in a note about CSS-in-JS.

CSS-in-JS was another pattern that is or was heavily used across projects at certain point in time. styled-components and Emotion are some of the popular CSS-in-JS libraries in the React community.

@lholmquist
Copy link
Member Author

Not wanting this to be added in on the docs here, but wanted to start a discussion on whether it makes sense to add in a note about CSS-in-JS.

If it is a pattern then i don't see why not

@lholmquist
Copy link
Member Author

@adimaniacal thanks for the spelling updates. this just proves how bad of a speller i am 🤣

@lholmquist
Copy link
Member Author

@adimaniacal i just added a small section related to the css-in-js pattern. mostly copy/paste of what you wrote :)

@lholmquist
Copy link
Member Author

lholmquist commented Apr 19, 2024

@paceaux i made some updates based on our latest meeting, mind taking a look

@lholmquist
Copy link
Member Author

@paceaux sorry for the second notification, but apparently in my above comment i had a space in between the @ symbol and the p, so i don't think you would have gotten the first notification 🤷

docs/development/styles.md Outdated Show resolved Hide resolved
docs/development/styles.md Outdated Show resolved Hide resolved

The use of [Media Queries](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_media_queries/Using_media_queries) allows you to apply CSS styles to your application using various characteristics, such as screen resolution and orientation. Media Queries will help with the responsive approach to writing applications as mentioned above.

Another way of making an application responsive is with either the Grid or Flex layouts. The team is familiar with both Grid and Flex, while Grid is slightly prefered. Depending on the situation, both can be used.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Another way of making an application responsive is with either the Grid or Flex layouts. The team is familiar with both Grid and Flex, while Grid is slightly prefered. Depending on the situation, both can be used.
Another way of making an application responsive is with either the [Grid](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout) or [Flexbox](https://developer.mozilla.org/en-US/docs/Learn/CSS/CSS_layout/Flexbox) layout methods. The team is familiar with both Grid and Flexbox, while Grid is slightly preferred. Depending on the situation, both can be used.
The most significant distinction between Grid and Flexbox, is that Grid is a two-dimensional layout method and Flexbox is a one-dimensional layout method.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine if we want to say Grid is slightly preferred, but if we are going to say that, we should explain why. Also, my opinion is that if your requirement is for a one-dimensional layout, then Flexbox is preferable because there's less to fiddle with.

lholmquist and others added 2 commits April 22, 2024 09:56
Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
@lholmquist lholmquist merged commit f5552f7 into nodeshift:main May 1, 2024
1 check passed
@lholmquist lholmquist deleted the start_of_styles branch May 1, 2024 15:24
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.

None yet

5 participants