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

Use h1 (19px) style for header instead of textXLarge (24px) #5835

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

aldo-expensify
Copy link
Contributor

@aldo-expensify aldo-expensify commented Oct 14, 2021

Details

Reduce size of headers to 19px following designs.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/181056

Tests / QA

  1. Create a NewDot account, verify it, log in

  2. Create a Workspace

  3. From the workspace menu:
    Screen Shot 2021-10-13 at 6 11 08 PM

  4. Enter in Issue corporate cards and check that the title Unlock free Expensify Cards has font size 19px
    Screen Shot 2021-10-13 at 6 12 10 PM

  5. Check the same thing for the following titles:

    1. Reimburse receipts section titles:
      • Capture receipts
      • Unlock next-day reimbursements
    2. Pay bills section titles:
      • Manage your bills
      • Unlock online bill payment
    3. Send invoices section titles:
      • Invoice clients and customers
      • Unlock online invoice collection
    4. Book travel section titles:
      • Unlock Concierge travel booking
    5. Connect bank account section titles:
      • Streamline payments
  6. Connect a VBA using the section (3) of this SO answer

  7. Some new titles will appear in the following sections, check their font size to be 19px:

    1. Issue corporate cards section titles:
      • Cards are ready! (post VBA-connected)
    2. Reimburse receipts section titles:
      • Fast reimbursements = happy members! (post VBA-connected)
    3. Pay bills section titles:
      • Hassle-free bills! (post VBA-connected)
    4. Send invoices section titles:
      • Money back, in a flash! (post VBA-connected)
    5. Book travel section titles:
      • Pack your bags! (post VBA-connected)
  8. During the process of adding a VBA, depending on the flow (try different options of the SO), we should encounter the titles:

    • Almost done!
    • Let’s finish in chat!
    • One more thing!
    • All set!
  9. Check that these titles also have font size 19px

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@aldo-expensify
Copy link
Contributor Author

With the proposed change, the following styles becomes unused:

App/src/styles/styles.js

Lines 144 to 149 in a77d468

textXLarge: {
color: themeColors.heading,
fontFamily: fontFamily.GTA_BOLD,
fontSize: variables.fontSizeXLarge,
fontWeight: fontWeightBold,
},

Should I remove it?

@shawnborton
Copy link
Contributor

shawnborton commented Oct 14, 2021 via email

@aldo-expensify aldo-expensify marked this pull request as ready for review October 14, 2021 01:30
@aldo-expensify aldo-expensify requested a review from a team as a code owner October 14, 2021 01:30
@aldo-expensify aldo-expensify changed the title [WIP] Use h1 (19px) style for header instead of textXLarge (24px) Use h1 (19px) style for header instead of textXLarge (24px) Oct 14, 2021
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team October 14, 2021 01:30
@aldo-expensify aldo-expensify self-assigned this Oct 14, 2021
Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

Clean, nice job!

@jasperhuangg jasperhuangg merged commit 5c38f0c into main Oct 14, 2021
@jasperhuangg jasperhuangg deleted the aldo_fix-title-font-size-19 branch October 14, 2021 19:16
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @jasperhuangg in version: 1.1.7-25 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@ogumen
Copy link

ogumen commented Oct 22, 2021

All of the mentioned visual headings are implemented as plain text using <div> tag. In order to make the overlay structure available to assistive technology users, the visual headings should be implemented using sequential headings semantics (starting with <h2> tag for main dialog heading, followed by <h3> - for sub-headings and so on). It is a failure of WCAG SC 1.3.1
Expensify_Visual headings implemented as plain text

@aldo-expensify
Copy link
Contributor Author

All of the mentioned visual headings are implemented as plain text using

tag. In order to make the overlay structure available to assistive technology users, the visual headings should be implemented using sequential headings semantics (starting with

tag for main dialog heading, followed by

- for sub-headings and so on). It is a failure of WCAG SC 1.3.1

Sounds good, we should create a new GH issue for this.

@ogumen
Copy link

ogumen commented Oct 22, 2021

@aldo-expensify, sure I can do it. @stitesExpensify informed me I should just comment the PR without creating a new issue.

@aldo-expensify
Copy link
Contributor Author

@aldo-expensify, sure I can do it. @stitesExpensify informed me I should just comment the PR without creating a new issue.

That may be right, sorry I wasn't saying that you should create it :), I was just stating that we should do that in a separate GH issue. I'll create the GH issue when I have some time.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

5 participants