-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Create Tag utils file #42559
Create Tag utils file #42559
Conversation
lol lint and jest have differing views on how to import this. it'll be fine once #42305 is merged and this is updated accordingly. |
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.
One NAB comment, and there are conflicts. Other than that LGTM 👍
if (!key) { | ||
return; | ||
} | ||
if (val === null || val === undefined) { |
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.
if (val === null || val === undefined) { | |
if (!val) { |
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.
I think this might be because val could be an empty string, which would still be false
? Not sure, this is just copied wholesale from Policy.ts
. I might leave it for now unless you disagree.
conflicts resolved! |
Reviewer Checklist
Screenshots/VideosN/A just moving code around Android: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.77-11 🚀
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.77-11 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.78-5 🚀
|
pendingFields: { | ||
name: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE, | ||
}, |
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.
We missed to spread the tag list here which caused #42917.
IMO, this was a big PR so it's just a missed case from the implementation
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.
Hi @allgandalf! So I actually didn't modify any code in this PR - I just moved it - so the missing spread looks like it came from the old file:
Not trying to pass the blame but more to help us find the original author!
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.
Umm, sorry for the Overlook, by the PR name create tag utils file
i assumed this was a new feature implementation :)
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.
All good!
errors: { | ||
[oldName]: oldName, | ||
[newName]: ErrorUtils.getMicroSecondOnyxError('workspace.tags.genericFailureMessage'), | ||
}, |
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 change caused this issue.
We don't need to pass errors
separately from tag objects
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.
hi! So this PR was just moving all the code from Policy.ts to Policy/Tag.ts - no changes in the code (that issue already existed prior to this). It'll be annoying to go through the history (sorry!) but we should try to find the real PR that caused the issue instead of this one.
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.
Oh, yeah
My fault, sorry 😅
I'll try to check history a little deeper
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.
all good! You're not the first (and it's confusing)
@luacmartins @amyevans
Details
Next stage of #38477 - will then make new PRs for Members, and Distance Rates still to come (Categories is here).
In terms of testing, we could maybe do some spot checks re: categories, but otherwise this is just administrative and shouldn't cause any issues. No new or changed functionality, just organization.
Fixed Issues
#38477
PROPOSAL:
Tests
Just play around a bit with tags (adding, deleting, renaming, requiring, etc.) and make sure it still works. This shouldn't have any effect on user experience).
Offline tests
QA Steps
No QA
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.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 and/or tagged@Expensify/design
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop