-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Larger mobile autocomplete fonts and paddings #1900
Larger mobile autocomplete fonts and paddings #1900
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded
Removed
Bigger
Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
cff0be7
to
d480374
Compare
b1bbbcf
to
f85806b
Compare
@trevdor Updated. :) Let me know if you want me to adjust the font sizes. |
Definitely easy tap targets now! Seems like the font-size could decrease just a hair on list items so they're smaller than group headings. That contrast was nice in your original screenshot. (Maybe our font sizes in On my phone, grasping the entire diff is tricky, so apologies if I've misread--but how about regular list items get weight 400, font-size...um, a rogue 17?!, and vertical passing of 8? (think I see 10 right now, and rows seem just a touch tall imo) I haven't had a chance to tweak it in my own browser to give a true "here's my favorite sizing"--it just feels a bit chunky on a small phone at the moment. Thanks for hearing me out! |
0b0ca3e
to
3833a74
Compare
@trevdor Adjusted the fonts and paddings. :) |
3833a74
to
17935f6
Compare
Trying to get through the rest of the code tonight, but the visual look good to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This will help me personally, if nothing else :D
Left a few questions/comments, but however you resolve those is good with me.
|
||
// It should not allow filtering on group names | ||
await userEvent.clear(input); | ||
await userEvent.type(input, 'Usual Expenses'); | ||
|
||
items = tooltip.querySelectorAll('[data-testid*="category-item"]'); | ||
items = tooltip.querySelectorAll('[data-testid$="category-item"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏🏻 yay for narrowing the selectors & the separating data-highlighted
in general
let highlighted = tooltip.querySelector('[data-highlighted]'); | ||
expect(highlighted).not.toBeNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're looking for one and not two highlights, how about?
let highlighted = tooltip.querySelector('[data-highlighted]'); | |
expect(highlighted).not.toBeNull(); | |
let highlighted = tooltip.querySelectorAll('[data-highlighted]'); | |
expect(highlighted).toHaveLength(1); |
(haven't run this change, so double-check me if you accept it)
@@ -63,7 +63,7 @@ test.describe('Transactions', () => { | |||
await expect(autocomplete).toMatchThemeScreenshots(); | |||
|
|||
// Select the active item | |||
await page.getByRole('button', { name: 'Clothing' }).click(); | |||
await page.getByTestId('Clothing-category-item').click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I prefer getByRole
with the user-visible text over grabbing elements by something the user can't see, in order to keep our tests behaving similarly to a user. Did the existing selector fail after the refactor?
As I say, it's a preference, so ship it as you see fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, the test is hanging on this getByRole
call so I had to change it. I'm happy to revert it back to use getByRole
if anybody can get it to work again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not worth holding it up for (although we're in code freeze at the moment).
d6f1353
to
d6e0817
Compare
@joel-jeremy To improve the readability of the selected category name, could you use a color with greater contrast? Thanks for increasing the font size in this component! |
I have updated the hover colors |
Thank you! Now it's perfect! |
* Larger mobile autocomplete fonts and paddings * Release notes * VRT + update tests * Update tests * Update data-highlighted and tests * Use styles text * Fix tests * Fix tests * Fix tests * Fix tests * Fix tests * Fix tests * Adjust Add Transaction padding + VRT updates * Larger autocomplete text and divider * Fix rebase * Fix rebase * Fix icons * Adjust fonts * Fix lint errors * PR feedback * VRT * Update embedded autocomplete highlight hover color * Refactor create payee button * Embedded create payee button color * Dummy change to re-run CI
Made autocomplete components easier to tap by making the fonts and paddings larger.
Previous:
New: