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

Update toolkit page subheadings #1999

Merged

Conversation

macho-catt
Copy link
Member

@macho-catt macho-catt commented Jul 22, 2021

Fixes #1479

What changes did you make and why did you make them ?

  • Placed suggest-guide-group div as a child of toolkit-flex-container for both the guides and external resources so that it becomes part of the grid, along with the guides
  • Set suggest-guide-group to span the entire column, which fixes the heading and button alignments so they align with the edges of the first and last guides
  • Removed "We made these" from subheading
  • Removed commented code and fixed some tab alignments
  • For external resources, wrapped the resources under a new div: external-resource-container. Without this div, the external resources sit to the left (instead of centered like in the figma) because of how grid works. To fix this, added media queries so that at certain breakpoints, this div is either a flex (above 1632px), a grid (below 1632px until above mobile) and back to flex (below mobile).
  • Added comment under _toolkit.scss to explain the above in the code:
    Refer to issue Update toolkit page subheadings #1999. This part, the accompanying divs in toolkit.html (lines 89 and 109),
    and $screen-desktop-large under layout.scss were added to take into account how the external
    resources look at a certain breakpoint ($bp-desktop-large-up). As of 7/22/2021, there are only three resources in the list.
    Since suggest-site-group div was added to the grid and takes up the entire first row, the rows that
    follow does not go to the center and instead aligns to the left. The media queries here fixes that.
    When there are four or more resources added in the future, this part (and the divs in toolkit.html)
    can be removed.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Full page before

toolkit-full-before

Full page after

toolkit-full-after

Guides responsiveness

guides

External Resources responsiveness

external

@github-actions github-actions bot added P-Feature: Toolkit https://www.hackforla.org/toolkit/ role: front end Tasks for front end developers Complexity: Medium labels Jul 22, 2021
@github-actions
Copy link

From your project repository, check out a new branch and test the changes.

git checkout -b macho-catt-fix-toolkit-subheading-1479 gh-pages
git pull https://github.com/macho-catt/website.git fix-toolkit-subheading-1479

@Aveline-art Aveline-art self-requested a review July 26, 2021 16:44
@Abel-Zambrano Abel-Zambrano self-requested a review July 29, 2021 18:49
abuna1985
abuna1985 previously approved these changes Jul 30, 2021
Copy link
Member

@abuna1985 abuna1985 left a comment

Choose a reason for hiding this comment

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

@macho-catt I was able to pull the changes and verify locally. It looks awesome. Thank you for your detailed pull request and comment to notate the addition of the $screen-desktop-large and implementation for the Toolkit page. Great work.

Copy link
Contributor

@Abel-Zambrano Abel-Zambrano left a comment

Choose a reason for hiding this comment

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

Hey, @macho-catt it's looking good, but there's still unalignment happening below 960px width. If you could please make changes so it's aligned with cards before it gets centered in mobile-screen.

@Abel-Zambrano
Copy link
Contributor

Screen.Recording.2021-07-29.at.6.45.33.PM.mov

@macho-catt
Copy link
Member Author

macho-catt commented Jul 30, 2021

@Abel-Zambrano good catch! I'll work on that, thanks!

@Aveline-art Aveline-art removed their request for review July 30, 2021 04:28
@macho-catt
Copy link
Member Author

New commits for @Abel-Zambrano's change request

What changes did you make and why did you make them ?

  • Under _toolkit.scss, fixed grid-template-column for below desktop view so that the header and the guides are still aligned on both edges
  • Made a new media query for below tablet view, and placed anything that uses below tablet view under that new media query
  • Added new class, external-resources-text for the external resources header because at a certain width below tablet view, it wraps the text and centers it, which makes it not aligned to the front edge. The new class aligns it to the left below tablet view.
  • Fixed scss indents

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

New full page (still looks the same)

full-page

Guides header responsiveness

first-header

External Resources header responsiveness

second-header

Copy link
Contributor

@Abel-Zambrano Abel-Zambrano left a comment

Choose a reason for hiding this comment

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

@macho-catt Wow! That looks great! Sorry, one more thing, I must've missed it. On file _toolkit.scss, can you move line 308's code for h2's margin-bottom into media query bp-below-mobile (line 314) so it can be aligned with the button on the right?

How it looks now

Screen Shot 2021-07-29 at 22 36 31

How it should look

Screen Shot 2021-07-29 at 22 37 12

@Abel-Zambrano
Copy link
Contributor

This change will also give the h2 and button some breathing room when viewed on mobile-screens

@macho-catt
Copy link
Member Author

Thanks @Abel-Zambrano

Changes

  • Fixed alignment so Guides and External Resources are aligned with the button
Fixed Header Alignment

header-alignment

Copy link
Contributor

@Abel-Zambrano Abel-Zambrano left a comment

Choose a reason for hiding this comment

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

Awesome work! @macho-catt

Copy link
Member

@abuna1985 abuna1985 left a comment

Choose a reason for hiding this comment

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

Great job @macho-catt. Thank you for your edits.

@abuna1985 abuna1985 merged commit eb388cf into hackforla:gh-pages Aug 1, 2021
@macho-catt macho-catt deleted the fix-toolkit-subheading-1479 branch August 5, 2021 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complexity: Medium P-Feature: Toolkit https://www.hackforla.org/toolkit/ role: front end Tasks for front end developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Toolkit Page subheadings
3 participants