Conversation
|
✔️ Deploy Preview for sb-apprentices ready! 🔨 Explore the source changes: 4b106c8 🔍 Inspect the deploy log: https://app.netlify.com/sites/sb-apprentices/deploys/61b2152e3697ee00083d1dc1 😎 Browse the preview: https://deploy-preview-31--sb-apprentices.netlify.app |
changangus
force-pushed
the
refactor--page-styles
branch
from
52f6dc8 to
443ffc4
Compare
jonoliver
left a comment
jonoliver
left a comment
There was a problem hiding this comment.
This is looking really good! Starting to add some screenshots of minor issues I noticed...
This screenshot was at 766px wide, looks like a spacing issue between your social links and Alice's photo:
| } | ||
| } | ||
|
|
||
| @mixin visually-hidden { |
changangus
force-pushed
the
refactor--page-styles
branch
from
443ffc4 to
d91f3b5
Compare
changangus
force-pushed
the
refactor--page-styles
branch
from
d91f3b5 to
c3621d7
Compare
- Created mixin for section padding - Added mixin to every section except ApprenticeQualities & PreviousApprentices - PreviousApprentices has custom padding due to the svg logo - gave PreviousApprentices a negative z-index because padding covers CurrentApprentices cmp - Refactored PreviousApprenticeGroups to align to have all groups align to the left - Refactored font sizes in the hero - Refactored PreviousApprenticeGroup to make sure triangle svg does not wrap - Moved visually hidden mixin from hero sass module to mixin partials file FSA21V2-156
changangus
force-pushed
the
refactor--page-styles
branch
from
c3621d7 to
9855501
Compare
| @use '../../styles/colors' as *; | ||
| @use '../../styles/spacing' as *; | ||
| @use '../../styles/fonts' as *; | ||
| @use '../../styles/mixins' as *; |
There was a problem hiding this comment.
I see the mixin being included here, but I don't see the section-padding mixin being used. Should that be included? It looks likes there are still some misalignments with this section:
styles/_mixins.scss
Outdated
| @@ -0,0 +1,32 @@ | |||
| @mixin section-padding { | |||
There was a problem hiding this comment.
It looks like the specificity of the media queries is overriding the intended vertical padding on the previous apprentices section. I'm also not sure that we want to couple the vertical padding to the breakpoints here. Let's decouple the concerns of vertical and horizontal spacing, and update this mixin so that it is only concerned with horizontal spacing. This involves 3 things:
- Explicitly set
padding-leftandpadding-rightinstead ofpadding - Move the
widthandmax-widthdeclarations into this mixin - Rename the mixin to
section-horizontal-spacing
From there, we should be able to set the previous apprentice padding without it being overridden. Subsequently, if the vertical padding needs to be shared across sections, we can create a section-vertical-spacing mixin (if necessary).
changangus
force-pushed
the
refactor--page-styles
branch
2 times, most recently
from
a3da8b8 to
fd8131c
Compare
- Abstracted horizontal and vertical padding into their own mixins - Created padding variables for mixins - Added new mixins to hero, current apprentices and call to action - add max width to hero container FSA21V2-156
- Fixed minor spacing issues with hero and cta art FSA21V2-156
changangus
force-pushed
the
refactor--page-styles
branch
from
1480542 to
4b106c8
Compare
3 participants
Description
FSA21V2-156
Spec
Designs: [link to design if applicable]
See Story: FSA21V2-156
To Test
npm run test:ciValidation
The following has been completed by the developer:
[delete anything irrelevant to this PR]
Browser Testing
[delete if irrelevant to this issue]
Gold Level Browsers
In these browsers, all content is accessible and design matches exactly. In most cases, bugs should be resolved before merge.
macOS
Windows
Mobile
Silver Level Browsers
In these browsers, all content is readable, but design may be inaccurate.
Windows
Mobile
Bronze Level Browsers
In these browsers, our only goal is content readability.
Windows