Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Superficial markup and style changes to App template #1472

Closed
wants to merge 0 commits into from

Conversation

lukejacksonn
Copy link
Contributor

This is a purely aesthetic suggestion that may be entirely dismissed (no offense taken!) if people deem it extraneous. I'm not precious about the design or implementation.. just an idea really, trying to change as little as possible.

Actions taken:

  • Flattens App-header and App-intro content (img, h2 and a p) into App
  • Adds min-height, flex-direction, padding and centered contents of App container
  • Adds box-sizing border-box to * selector
  • Adds responsive font-size, line-height and color to body

Before:

screen shot 2017-01-29 at 12 25 25 am

After:

screen shot 2017-01-29 at 12 21 58 am

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please # at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

.App-logo {
animation: App-logo-spin infinite 20s linear;
height: 80px;
.App > *+* {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to avoid selectors like this. We are generally trying to encourage people to "think in components", and any more fancy selectors often break when you nest one component into another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can re-write this behavior in many ways or explicitly put margins on each child of the component. I will go ahead and do that!

As a side note I have always found this selector very useful, especially for container components. I think an axiomatic approach to layout complements very nicely the principles of react. Once you get your head around the magic it makes sense. See lobotomized owls.

I am a very poor mathematician, but I have a great fondness for Euclid’s postulates: a set of irreducible rules, or axioms, that form the basis for complex and beautiful geometries. Thanks to Euclid, I understand that even the most complex systems must depend on foundational rules, and CSS is no different. Although modularization of a complex interface is a necessary step in its maturation, any interface that does not follow basic governing tenets is going to lack clarity.

The owl selector allows you to control flow content, but it is also a way of relinquishing control. By styling elements according to context and circumstance, we accept that the structure of content is—and should be—mutable. Instead of prescribing the appearance of individual items, we build systems to anticipate them.

}

.App-intro {
font-size: large;
code {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, this will behave unexpectedly when you nest components. We use class names everywhere to avoid this problem. Don't style tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will style this with a class 👍

@@ -1,5 +1,12 @@
* {
box-sizing: border-box;
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this one. But maybe we should just bring in some good CSS reset by default? Can you suggest any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends how involved you want it to be really..

The usual suspect resets are either:

  • normalize.css about setting sensible defaults to reduce browser inconsistencies
  • reset.css. more about stripping browser defaults leaving everything bare bones.

Alternatively we could extend on this to be something like:

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

body {
  font-family: sans-serif;
  font-size: 16px;
  line-height: 1;
}

Which achieves a pretty sufficient (if not slightly brutal) reset that is easy to comprehend and easy to extend. Without employing a dependency.

What do you think?

Copy link
Contributor

@Timer Timer Feb 12, 2017

Choose a reason for hiding this comment

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

How do you feel about Bootstrap's Reboot? It's an extension of normalize.css.

Copy link
Contributor Author

@lukejacksonn lukejacksonn Feb 12, 2017

Choose a reason for hiding this comment

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

I would consider it essentially, a relative to normalize. Better docs perhaps

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I've seen normalize used on a lot of projects. It doesn't handle box sizing, so I typically refer to Paul Irish's canonical source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, I just wrote * { ... } for succinctness!

@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants