-
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
Add "Create Tag" page #38158
Add "Create Tag" page #38158
Conversation
@ishpaul777 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-03-13.at.1.41.31.AM.movAndroid: mWeb ChromeRecord_2024-03-13-02-00-07.mp4iOS: NativeRPReplay_Final1710274708.MP4iOS: mWeb SafariScreen.Recording.2024-03-13.at.2.35.44.AM.movMacOS: Chrome / SafariScreen.Recording.2024-03-13.at.1.37.51.AM.movMacOS: DesktopScreen.Recording.2024-03-13.at.2.03.01.AM.mov |
src/languages/es.ts
Outdated
tagRequiredError: 'Tag name is required.', | ||
existingTagError: 'A tag with this name already exists.', | ||
invalidTagName: 'Invalid tag 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.
Can you please ask for translation in slack?
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.
Yes, my bad 😄
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.
still waiting for translations
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@mountiny What should be the offline behaviour here? shouldn't we greyout the newly created tag ? Screen.Recording.2024-03-13.at.12.27.23.AM.mov |
Not within the scope of this ticket. This ticket does not involve the list that displays the tags. |
I think so and I do think it's the scope here since we'd add the pendingAction to the call to createPolicyTag here |
This comment was marked as duplicate.
This comment was marked as duplicate.
Same question for where do we show the error for failure scenario. What is expected ? |
It would / should show up as a red brick below the list item. Again, I think this should be picked up from the |
src/libs/actions/Policy.ts
Outdated
Tag: { | ||
errors: 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.
for failure scenario shouldn't we remove the tag from the onyx?
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 don't think we do that. We just show an error and once the user clicks on the x
beside the red brick, only then we remove it.
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.
Once the X button is clicked confirming the user read the error the tag should be removed
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.
makes sense.. but i dont see the error appear yet in this pattern do you think that out of scope too?
cc @luacmartins
accessibilityLabel={translate('common.name')} | ||
inputID={INPUT_IDS.TAG_NAME} | ||
role={CONST.ROLE.PRESENTATION} | ||
autoFocus |
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.
autofocus
prop not works as expected on android and keyboard is not popup. can you try https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#how-do-i-auto-focus-a-textinput-using-usefocuseffect for autofocus instead
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.
Record_2024-03-13-01-06-09.mp4
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 also have useAutoFocusInput
hook that has similar implementation as in the docs, maybe use that ?
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.
Handled!
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!
Hm... This is similar to how categories page is behaving. @mountiny / @luacmartins do we need any change here? |
interesting, I would say we dont have to worry about it now and handle it together with Categories page as a follow up if the issue is same for both |
Last one above then we are good tag design team for final review... (new checklist item)
|
@shawnborton This is ready for a final look, C+ approved, could you have a look please? |
We're waiting for translations as well @mountiny |
Looks good to me, thanks! |
@allroundexperts Your videos seems to be mismatched with platforms can you please fix that |
src/libs/actions/Policy.ts
Outdated
onyxMethod: Onyx.METHOD.MERGE, | ||
key: `${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`, | ||
value: { | ||
Tag: { |
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 don't think this is accurate. A user can rename the tagList name and this request would fail. We need to dynamically update this name. Same goes for the other cases below
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.
Any idea how to get that name? We can have multiple items in a tag list. How would that be handled?
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.
Moving this discussion over to Slack.
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.
Handled!
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.
Looking good so far +1 to Carlos's comments, but I think we have to align on the policy terminology
src/languages/es.ts
Outdated
tagRequiredError: 'Tag name is required.', | ||
existingTagError: 'A tag with this name already exists.', | ||
invalidTagName: 'Invalid tag 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.
still waiting for translations
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
Co-authored-by: Carlos Martins <luacmartins@gmail.com>
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Tests passed when I merged. |
✋ 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/luacmartins in version: 1.4.52-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.52-6 🚀
|
Details
This PR adds the ability to add a new tag to the workspace in ND.
Fixed Issues
$ #37311
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
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
Screen.Recording.2024-03-12.at.10.59.28.PM.mov
Android: mWeb Chrome
Screen.Recording.2024-03-12.at.10.48.42.PM.mov
iOS: Native
Screen.Recording.2024-03-12.at.11.02.44.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-03-12.at.10.44.28.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-03-12.at.10.40.11.PM.mov
MacOS: Desktop
Screen.Recording.2024-03-12.at.10.42.44.PM.mov