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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web/flexbox utils, login style rework #39

Merged
merged 4 commits into from Aug 9, 2018

Conversation

@bobheadxi
Copy link
Contributor

bobheadxi commented Aug 9, 2018

Closes #36

馃懛 Changes

Alright, so this ended up being a bit bigger than expected. Recommend reading this for the rationale behind my changes and what we should do with these going forward.

  • Added .flex in utils.sass - these are the flexbox utilities i mentioned
  • Added breakpoints.sass, though these aren't in use yet. just to get the ball rolling on responsive design

Then I started looking into getting these implemented as an example, and noticed that the Login page's style is super specific... guess it didnt seem too bad in the review. So I:

  • moved text styles into global a, h2, p - hopefully this helps with uniformity
  • moved padding, margin, width/height styles into global classes in utils.sass
  • added decorations.sass for decorative styles, like shadows - other classes will need to be updated to use these
  • completely gutted Login style to reduce its complexity and utilize the existing classes as much as possible

The new stylesheet only declares constraints and super specific styles:

div#login
  > div
    max-width: 748px
    width: 60%

    form
      height: 60%
      min-height: 400px

      > .card
        > .card-right
          background: linear-gradient(180deg, #2C31AE 0%, #21258A 100%)

The resultant, clean, declarative JSX:

      <div id="login" className="fill-height flex jc-center ai-center dir-col">
        <div>
          <form
            onSubmit={this.login}
            className="input-group">
            <h2 className="pad-sides">Sign in</h2>
            <div className="card split flex">
              <div className="pad-ends pad-sides margin-text-inputs">
                <TextInput
                  placeholder="Enter your email"
                  value={email}
                  onChange={this.onEmailChange}
                  error={error}
                  label="Email"
                  id="email"
                />
                <PasswordInput
                  placeholder="Enter your password"
                  value={password}
                  onChange={this.onPasswordChange}
                  error={error}
                  label="Password"
                  id="password"
                  showErrorMessage
                />
                <PrimaryButton
                  text="Submit"
                  className="fill-width"
                  />
                <p className="flex jc-center">
                  Don&apos;t have an account yet?&nbsp;
                  <Link to="/">Apply here</Link>
                </p>
              </div>
              <div className="card-right" />
            </div>
          </form>
        </div>
      </div>

馃挱 Notes

The goal with this is to reduce style duplication as much as possible, and achieve a more uniform look. If all our p have the same margins, it would be far easier to set up new elements that look like they fit in, for example.

This goes especially in padding - nothing looks weirder than padding that varies like crazy, so having a set of padding classes to choose from is a good idea. Plus, this means it'll be easier to implement responsiveness - for example, all classes with a certain padding class should lose/reduce their padding at the same breakpoints.

So why do all this instead of fancy, deeply nested styles that depend on a very specific hierarchy of elements? Because as soon as you move anything, say, one level of nesting deeper:

image

It also obscures a lot of the style logic, and makes it very hard to find what's doing what. More declarative classes = clearer style structures = happier time maintaining and adding to this stuff later on.

Also, just less styles in general. If additional variations are needed, just add subclasses.

For another example, see #41

馃敠 Testing Instructions

make web, everything should look the same

@bobheadxi bobheadxi requested review from mingyokim and chamkank Aug 9, 2018
@bobheadxi bobheadxi assigned bobheadxi and unassigned bobheadxi Aug 9, 2018
@bobheadxi bobheadxi added this to In progress in nwHacks 2019 via automation Aug 9, 2018
@bobheadxi bobheadxi moved this from In progress to Needs review in nwHacks 2019 Aug 9, 2018
@bobheadxi bobheadxi added the :web label Aug 9, 2018
@bobheadxi bobheadxi mentioned this pull request Aug 9, 2018
1 of 1 task complete
nwHacks 2019 automation moved this from Needs review to Reviewer approved Aug 9, 2018
Copy link
Contributor

mingyokim left a comment

looks good! thanks for doing this, i really like where this is going. it made the stylesheets a LOT cleaner.

@@ -0,0 +1,62 @@
.fill
&-width

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 9, 2018

Contributor

wtf i didnt know you could do this

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 9, 2018

Author Contributor

There is a bit of a gotcha though: I don't think parent class traits propagate to child classes using this syntax, so anything defined under fill needs to be applied using the @extends directive on each child class

Could be wrong on this though


// padding declarations
.pad
&-sides

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 9, 2018

Contributor

I think we might want to be more specific here like pad-sides-xl so that we can vary the size of padding, but we can that on an ongoing basis

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 9, 2018

Author Contributor

Yeah, if we see the need for more we should add them on an ongoing basis - not sure if this is xl or not quite yet 馃構

@bobheadxi bobheadxi mentioned this pull request Aug 9, 2018
0 of 3 tasks complete
@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Aug 9, 2018

@chamkank any thoughts on this? do you think it's good to go?

Since this affects all of us, would like to make sure we all get an approval in 馃憤

<button
onClick={onClick}
type="submit"
disabled={disabled}
className="primary">
className={`primary ${className}`}>

This comment has been minimized.

Copy link
@chamkank

chamkank Aug 9, 2018

Member

Extensible 馃槏

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Aug 9, 2018

Thanks guys!

@bobheadxi bobheadxi merged commit 574ba19 into master Aug 9, 2018
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
nwHacks 2019 automation moved this from 馃弳 Approved to 馃梼 Closed Aug 9, 2018
@bobheadxi bobheadxi deleted the web/flexbox-utils branch Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
nwHacks 2019
  
馃梼 Closed
3 participants
You can鈥檛 perform that action at this time.