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

[Teach-298] Create section - Update dialog styling #50605

Merged
merged 6 commits into from Mar 10, 2023
Merged

[Teach-298] Create section - Update dialog styling #50605

merged 6 commits into from Mar 10, 2023

Conversation

levadadenys
Copy link
Collaborator

@levadadenys levadadenys commented Mar 6, 2023 •
edited

[Teach-298] Create section - Update dialog styling

Restyling(Visual appearance of pr) is already approved by Molly Peredo

Before:
image

After:

image

image

image

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@levadadenys levadadenys changed the title Denys/teach/teach 298 [Teach-298] Create section - Update dialog styling Mar 7, 2023
@levadadenys levadadenys requested review from a team March 7, 2023 17:03
@levadadenys levadadenys marked this pull request as ready for review March 7, 2023 17:06
rshipp
rshipp reviewed Mar 7, 2023
View reviewed changes
apps/src/templates/teacherDashboard/LoginTypePicker.jsx
Comment on lines -115 to +124
<Heading1>{title}</Heading1>
<Heading2>{i18n.addStudentsToSectionInstructionsUpdated()}</Heading2>
<Heading3>{title}</Heading3>
Copy link
Contributor

Choose a reason for hiding this comment

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

i can't find a definitive answer on what starting heading level inside a modal is best for screenreaders, but i would guess that h2 or h1 would be easier to navigate. would the Heading3s in here work as Heading2 or Heading1 with a class to adjust the styling?

that said, this modal already has other major accessibility issues, so I wouldn't consider this a blocking comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why h3 should be a problem for screenreader. h3 is still a header so I don't see any semantic issues here, correct me if I'm wrong. In my opinion it's better to stick with one type-specs that we agreed and put in this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As an option - we can discuss it with designer, Molly Peredo (Looks like she doesn't have a githun account)

Copy link
Contributor

Choose a reason for hiding this comment

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

this page has some best practices: https://usability.yale.edu/web-accessibility/articles/headings#using. ideally, headings should only be chosen for their semantic meaning, and not based on style. it sounds like there's larger design questions here that make this outside the scope of this PR, so I can ask Molly about this separately. thanks for that link!

@rshipp rshipp requested a review from a team March 7, 2023 17:37
rshipp
rshipp approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@rshipp rshipp left a comment

Choose a reason for hiding this comment

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

started a convo with Molly about my heading questions, but I don't think that should block this PR. the rest looks good to me!

apps/src/templates/teacherDashboard/ParticipantTypePicker.jsx
<ParticipantTypeCard
onClick={setParticipantType}
key={index}
key={type}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!

apps/src/templates/teacherDashboard/LoginTypeCard.jsx
this.setState({hover: !this.state.hover});
};
// Toggle hover is not working properly when component is rendered with mouse already 'over' the component.
// In order for hover to work properly - set on/off hover implicitly.
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for this comment!

bethanyaconnor
bethanyaconnor reviewed Mar 10, 2023
View reviewed changes
apps/src/lib/ui/Headings.jsx
fontFamily: '"Gotham 5r", sans-serif',
fontSize: 16,
lineHeight: '24px'
fontFamily: '"Barlow Semi Condensed Semibold", sans-serif',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking: will this affect any other components unexpectedly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've checked, it should only affect PR

bethanyaconnor
bethanyaconnor approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@bethanyaconnor bethanyaconnor left a comment

Choose a reason for hiding this comment

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

🎉

@levadadenys levadadenys merged commit 2b1ce97 into staging Mar 10, 2023
@levadadenys levadadenys deleted the denys/teach/teach-298 branch March 10, 2023 17:38
bethanyaconnor added a commit that referenced this pull request Mar 10, 2023
@bethanyaconnor
Revert "[Teach-298] Create section - Update dialog styling (#50605)"
b90ecd3 
This reverts commit 2b1ce97.
@bethanyaconnor bethanyaconnor mentioned this pull request Mar 10, 2023
Merged
bethanyaconnor added a commit that referenced this pull request Mar 10, 2023
@bethanyaconnor
Revert "[Teach-298] Create section - Update dialog styling (#50605)" (#…
1ee76e5 
…50707)

This reverts commit 2b1ce97.
levadadenys added a commit that referenced this pull request Mar 19, 2023
@levadadenys
Revert "Revert "[Teach-298] Create section - Update dialog styling (#…
be23f8e 
…50605)" (#50707)"

This reverts commit 1ee76e5.
@levadadenys levadadenys mentioned this pull request Mar 19, 2023
Merged
8 tasks
levadadenys added a commit that referenced this pull request Mar 22, 2023
@levadadenys
[Teach-298] Create section - Update dialog styling [Rework/Fixed] (#5…
3482c75 
…0842)

* Revert "Revert "[Teach-298] Create section - Update dialog styling (#50605)" (#50707)"

This reverts commit 1ee76e5.

* add possibility to use rebranded heading3 if needed

* * fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rshipp rshipp rshipp approved these changes

@bethanyaconnor bethanyaconnor bethanyaconnor 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.

None yet
3 participants
@levadadenys @rshipp @bethanyaconnor