-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Design follow up to Tag and Category lists #30264
Design follow up to Tag and Category lists #30264
Conversation
…ture/29574-category-long-name
…ture/29574-category-long-name
…ture/29574-category-long-name
The code looks good! @rezkiy37 Do I need to mock the categories so they can be nested and have the long name? |
@mollfpr, I can invite you to an already prepared workspace. You will be able to play with categories via OldDot, so feel free to modify them. I need your login to send an invite 🙂 Also, please ask for a few betas: |
@rezkiy37 Thank you! Here's the email test.luthfi.002@gmail.com |
@mollfpr, just sent, please check it out 😉 |
Reviewer Checklist
Screenshots/Videos |
…ture/29574-category-long-name
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.
LGTM and tests well 👍
@dubielzyk-expensify @dannymcclain could you please give this one a final design check? Thank you! |
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.
This is looking good to me based on the screenshots! If ya wanna invite me to the demo-mega-long-categories workspace I'd be happy to check it out locally too (danny@expensify.com). But I think it's feeling like what Shawn proposed.
@dannymcclain, just sent an invite to Collect Policy workspace 🙂 |
@rezkiy37 woohoo thanks! Just checked it out and I think still think it's looking good 😊 |
@mountiny, we've received the approvals. Seems like we can move forward here. |
…ture/29574-category-long-name
I have to inform that starting tomorrow I have a short vocation until next Monday (6.11.2023). Feel free to left any comments, I will address them. See you soon 😉 Btw, I've synced with the latest |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.94-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.94-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.95-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.95-9 🚀
|
Details
This PR introduces a few design improvements to Tag and Category lists:
Requirements - #29574 (comment).
Fixed Issues
$ #29574
PROPOSAL: N/A
Tests
Category list with a nested long named categories
Category/Tag lists with sections
Selected long named category/tag wraps up to 2 lines on the confirm page.
Offline tests
Same as "Tests".
QA Steps
Same as "Tests".
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Confirm Page Long Category and Tag
Nesting
Search
Section Spaces
Android: mWeb Chrome
Confirm Page Long Category and Tag
Nesting
Search
Section Spaces
iOS: Native
Confirm Page Long Category and Tag
Nesting
Search
Section Spaces
iOS: mWeb Safari
Confirm Page Long Category and Tag
Nesting
Search
Section Spaces
MacOS: Chrome / Safari
Confirm Page Long Category and Tag
Nesting
Search
Section Spaces
Small List
MacOS: Desktop
Confirm Page Long Category and Tag
Nesting
Search
Section Spaces