-
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
Allow for multiple levels of tags on workspace #34983
Allow for multiple levels of tags on workspace #34983
Conversation
…ture/33983-multiple-levels-tags
…ture/33983-multiple-levels-tags
…ture/33983-multiple-levels-tags
…ture/33983-multiple-levels-tags
…ture/33983-multiple-levels-tag
Resolved the conflicts. |
@rezkiy37 more conflicts and now there's a typescript error :( Feel free to ping me on Slack when you resolve and hopefully, I can just review/merge right away. |
…ture/33983-multiple-levels-tags
✋ 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/yuwenmemon in version: 1.4.43-0 🚀
|
data: { | ||
tagName: key, | ||
}, |
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 broke the client side violation logic for tags -- I'm fixing it. Why did we add why did we add {data { tagName }}
here, anduserMessage
?
Update data.tagName is necessary to show violations where the tag list name has a custom name, eg, Missing Department
Update 2 it looks like userMessage was already there when this PR was implemented
It's being fixed here #36919 |
@cead22 That seems a totally different deploy blocker 😄 |
I think you are fixing it here since it mentions the tag violation is not showing. |
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.43-20 🚀
|
function getTagListName(policyTags: OnyxEntry<PolicyTags>) { | ||
if (Object.keys(policyTags ?? {})?.length === 0) { | ||
return ''; | ||
function getTagLists(policyTagList: OnyxEntry<PolicyTagList>): Array<PolicyTagList[keyof PolicyTagList]> { |
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.
Heads up that using indexed types in this manner goes against our TS style guidelines.
Instead of PolicyTagList[keyof PolicyTagList]
, we should use the much more readable ValueOf
utility from type-fest
:
function getTagLists(policyTagList: OnyxEntry<PolicyTagList>): Array<ValueOf<PolicyTagList>> {
I'm tempted to create an issue to create a lint rule to enforce this and automatically update it via Prettier, but we have more urgent matters at hand.
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.
Thank you for this comment. I will use ValueOf
next time 🙂
Details
The PR introduces a new logic for using multiple levels of tags. It supports for creating/editing requests with independent multi-level tags for paid workspaces.
Fixed Issues
$ #33983
PROPOSAL: N/A
Tests
Pre-condition:
Apply tags for creating an expense report
Apply tags for editing an expense report
Apply tags for a split bill
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.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
Android.1.mp4
Android.2.mp4
Android.3.mp4
Android: mWeb Chrome
Android.Chrome.1.2.mp4
Android.Chrome.3.mp4
iOS: Native
IOS.1.mp4
IOS.2.mp4
IOS.3.mp4
iOS: mWeb Safari
IOS.Safari.1.mp4
IOS.Safari.2.mp4
IOS.Safari.3.mp4
MacOS: Chrome / Safari
Chrome.1.mp4
Chrome.2.mp4
Chrome.3.mp4
MacOS: Desktop
Desktop.1.mp4
Desktop.2.mp4
Desktop.3.mp4