Skip to content

Chisel apprentice qualities#33

Merged
changangus merged 5 commits intomainfrom
chisel--apprentice-qualities
Dec 15, 2021
Merged

Chisel apprentice qualities#33
changangus merged 5 commits intomainfrom
chisel--apprentice-qualities

Conversation

@auaruss
Copy link
Contributor

@auaruss auaruss commented Dec 9, 2021

Description

chisel: fix initial spacing and styling for apprentice-qualities component

Spec

Design

See Story: FSA21V2-164

To Test

  1. Make sure all PR Checks have passed (Github Actions, Netlify etc).
  2. Pull down all related branches.
  3. Confirm all tests pass: npm run test:ci

[For an example of good validation instructions, check out Bryan's Bouncy Ball PR.]

Validation

The following has been completed by the developer:

  • This PR has code changes, and our linters still pass.
  • This PR affects production code, so it was browser tested (see below).

Browser Testing

Gold Level Browsers

In these browsers, all content is accessible and design matches exactly. In most cases, bugs should be resolved before merge.

macOS

  • Chrome, current release
  • Firefox, current release
  • Safari, current release

Windows

  • Chrome, current release
  • Firefox, current release

Mobile

  • Chrome on Android 9
  • Safari, current release, on iPhone

Silver Level Browsers

In these browsers, all content is readable, but design may be inaccurate.

Windows

  • Edge, current release

Mobile

  • Android 8 - Chrome on Phone
  • Safari, version before current release, on iPad (including various split screen widths)
  • Chrome on iPhone

Bronze Level Browsers

In these browsers, our only goal is content readability.

Windows

  • IE 11

@netlify
Copy link

netlify bot commented Dec 9, 2021

✔️ Deploy Preview for sb-apprentices ready!

🔨 Explore the source changes: 11dd67c

🔍 Inspect the deploy log: https://app.netlify.com/sites/sb-apprentices/deploys/61ba1023915fe70009aa70ce

😎 Browse the preview: https://deploy-preview-33--sb-apprentices.netlify.app

@auaruss auaruss force-pushed the chisel--apprentice-qualities branch 3 times, most recently from fa9a990 to 63c925b Compare December 13, 2021 15:01
@changangus changangus force-pushed the chisel--apprentice-qualities branch 4 times, most recently from 28b548b to ed5f735 Compare December 14, 2021 16:05
&__main {
margin: auto;
@include horizontal-section-spacing();
box-sizing: border-box;
Copy link
Member

Choose a reason for hiding this comment

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

This is looking great Alice, I think I figured out a simple way to stop the text from floating off to the right on larger screens.

Could we add this CSS to &__main?

    justify-content: right;
    max-width: $max-content-width + ($xlarge-screen-padding * 2);
    margin: auto;

☝️ Setting the max-width this way feels a bit hacky, but absent a bigger refactor to the DOM structure, it it's a simple way to account for the padding with border-box setting.

&__main {
margin: auto;
@include horizontal-section-spacing();
box-sizing: border-box;
Copy link
Member

@jonoliver jonoliver Dec 14, 2021

Choose a reason for hiding this comment

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

This is looking great Alice, I think I figured out a simple way to stop the text from floating off to the right on larger screens. Here's a screenshot for reference:

Screen Shot 2021-12-14 at 3 22 55 PM

Could we add this CSS to &__main?

    justify-content: right;
    max-width: $max-content-width + ($xlarge-screen-padding * 2);
    margin: auto;

☝️ Setting the max-width this way feels a bit hacky, but absent a bigger refactor to the DOM structure, it it's a simple way to account for the padding with border-box setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

Copy link
Member

@jonoliver jonoliver left a comment

Choose a reason for hiding this comment

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

Thanks for the updates Alice! Nice work!

@auaruss auaruss force-pushed the chisel--apprentice-qualities branch from bd7a6e1 to 11dd67c Compare December 15, 2021 15:56
@changangus changangus merged commit 11dd67c into main Dec 15, 2021
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.

3 participants