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

Standardizing wins page #1204

Merged
merged 19 commits into from
Mar 17, 2021
Merged

Conversation

evancchen
Copy link
Contributor

Fixes #1166

@evancchen evancchen requested a review from akibrhast March 14, 2021 18:36
@akibrhast akibrhast requested a review from Lol-Whut March 14, 2021 19:44
@Lol-Whut
Copy link
Member

Lol-Whut commented Mar 15, 2021

It looks like element nesting got shuffled around a little bit during edits. When I pulled the branch to my machine and accepted incoming changes, I got syntactical error warnings for the curly braces on lines 56 and 87, and styling changes did not show up for me in the browser. @akibrhast are you seeing the same, or is this on my end?

For reference, right here it looks like there's an extra curly brace:
image

Copy link
Member

@akibrhast akibrhast left a comment

Choose a reason for hiding this comment

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

@evancchen

  • Please fix the merge conflicts.
  • Please ensures scss nests are properly nested and you can build the changes locally after addressing the merge conflicts

This is what I am seeing on my end when I try to build with docker-compose up

hfla_site    | jekyll 3.9.0 | Error:  Invalid CSS after "...b67d488000a5e71": expected "{", was "}" on line 59
hfla_site    |   Conversion error: Jekyll::Converters::Scss encountered an error while converting 'assets/css/main.scss':
hfla_site    |                     Invalid CSS after "...b67d488000a5e71": expected "{", was "}" on line 59
hfla_site    | /usr/gem/gems/jekyll-sass-converter-1.5.2/lib/jekyll/converters/scss.rb:123:in `rescue in convert': Invalid CSS after "...b67d488000a5e71": expected 
"{", was "}" on line 59 (Jekyll::Converters::Scss::SyntaxError)

@evancchen evancchen requested a review from akibrhast March 16, 2021 00:19
@akibrhast
Copy link
Member

@Lol-Whut It seems like @evancchen has updated the PR since our last review. Please re-review this and approve his changes if everything looks good , and nothing breaks.

@Lol-Whut
Copy link
Member

This is definitely looking better! No merge problems, the h1 tag is giving the "Let's Celebrate Together!" its styling and the margin-bottom is now 56px. However, it looks like the max-height of the .wins-hero img class still needs to be adjusted to 360px and the color of .wins_see-more-div still needs to be adjusted to $color-red. I think once those changes are in, it'll be perfect!

I think we should be seeing those two changes in these areas:
image

Copy link
Member

@Lol-Whut Lol-Whut left a comment

Choose a reason for hiding this comment

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

I think once the max-height and color changes I mentioned in my most recent comment are made, we'll be good to go! @evancchen

@evancchen evancchen requested a review from Lol-Whut March 17, 2021 00:43
@@ -6,14 +6,10 @@
align-items: center;
justify-content: space-evenly;
img{
height: 366;
height: 360px;
Copy link
Member

Choose a reason for hiding this comment

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

change this to max-height and it'll be perf!

@Lol-Whut
Copy link
Member

Looks good to me!

@Lol-Whut Lol-Whut merged commit 43104d4 into hackforla:gh-pages Mar 17, 2021
@evancchen evancchen deleted the standardizing-wins-page branch March 20, 2021 16:33
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.

Standardize Wins Page
3 participants