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.

Sign up for GitHub

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

Jump to bottom

Enhancement/88 implement new design #90

Closed
wants to merge 5 commits into from
Closed

Enhancement/88 implement new design #90

wants to merge 5 commits into from

Conversation

ashaduzzamane
Copy link
Member

@ashaduzzamane ashaduzzamane commented Oct 31, 2020 •
edited
Loading

Tickets:

List of changes:

-Updated the designs/images of the sponsor and about sections
-Changed the button colours
-Added a grey background to the sections
-Added Purple wire at the bottom of the sponsor section

image
image

Type of change:

  • Bug fix
  • New feature
  • Breaking change
  • New release
  • Requires a documentation update

How did you do this?

Why are you choosing this approach?

Questions for code reviewers?

PR Checklist:

  • Merged develop branch (before testing)
  • Ran yarn format to format code
  • Listed change(s) in the Changelog
  • Tested all links in project relevant browsers
  • Tested all links on different screen sizes
  • Referenced all useful info (issues, tasks, etc)

@ashaduzzamane ashaduzzamane requested a review from logan-r October 31, 2020 18:40
loreina
loreina reviewed Oct 31, 2020
View reviewed changes
Copy link
Member

@loreina loreina left a comment

Choose a reason for hiding this comment

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

reviewed, looks nice! I would say if there's time, to convert measurements from px to rem where applicable to help with responsiveness.

@logan-r are we planning on using a mchacks8 repo for this upcoming hackathon site? would be nice if we can retain the old mchacks7 site here

src/components/Bg/BgStyles.jsx Outdated
@@ -7,7 +7,7 @@ export const BgStyles = styled.section`
position: relative;
margin-bottom: 60px;
margin-top: 100px;
background: url(${BgImg});
background: #F4F4F4;
Copy link
Member

Choose a reason for hiding this comment

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

we should have a colour variable defined for this

Copy link
Member Author

Choose a reason for hiding this comment

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

we should have a colour variable defined for this

I can create a variable. I thought it wasn't worth it since we're only gonna use this colour once. what do u guys think?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should have the entire grayscale as defined color variables if not there already

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, colors should be same from dashboard to static site, no hardcoded colors

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix

@ashaduzzamane ashaduzzamane requested a review from loreina November 1, 2020 04:08
loreina
loreina reviewed Nov 1, 2020
View reviewed changes
Copy link
Member

@loreina loreina left a comment

Choose a reason for hiding this comment

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

Please fill out the PR checklist/template and name your PR accordingly

src/components/variable.jsx Outdated
@@ -4,6 +4,8 @@ export const colorHackRedMed = "#f56f65"
export const colorHackRedLight = "#f89790"
export const colorHackYellow = "#ffd081"
export const colorHackTeal = "#48DEE2"
export const purple = '#5C63AB'
export const purpleLight = '#5C63AB19'
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 not sure if this hex code is accurate?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, apparently the last two are alpha (transparency)

@logan-r
Copy link
Member

logan-r commented Nov 1, 2020 •
edited
Loading

This looks really good!! @ashaduzzamane
Can we increase the horizontal padding between the images and the text a bit, it's a bit on the squashed side
(edit: this is mostly between the laptop and the sponsor block, feel like padding looks different between the two blocks?)

@loreina Yeah, I'll set that up Monday or Tuesday then revert this repo back to end of 2020 site

logan-r
logan-r approved these changes Nov 22, 2020
View reviewed changes
Copy link
Member

@logan-r logan-r left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@logan-r
Copy link
Member

logan-r commented Nov 22, 2020

Closing to move to mchacks 8 repo

@logan-r logan-r closed this Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loreina loreina loreina left review comments

@logan-r logan-r logan-r approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashaduzzamane @logan-r @loreina