Add missing multi tag view #41351
Add missing multi tag view #41351
Conversation
|
@ikevin127 is going to jump on this for us. Thanks! |
|
@s77rt did you ensure that if there's only one level of tag, the UI remains as it is today in production on the Tags page? |
|
@trjExpensify Yes that area is not touched |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
This comment was marked as resolved.
This comment was marked as resolved.
|
Nice work picking this up! It's testing quite well for me on that adhoc build: 2024-05-01_00-09-32.mp4Few things to note:
CC: @dubielzyk-expensify @JmillsExpensify
|
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safari *Screen.Recording.2024-04-30.at.20.02.16.movMacOS: Desktop |
There was a problem hiding this comment.
I tested #41279 expected results steps:
- ✅ When more than one of
Classes,Customers/ProjectsorLocationsare selected for import, the Tags page table will represent each distinct tag level imported from QBO. - ✅ If the tag level is required,
Requiredwill be added to the status column in the table instead ofEnabled||Disabled. - ✅ Behind the "Settings" button on the
Tagspage header in the RHP:
3a. The toggle for "Members must tag all spend" will remain, allowing users to bulk toggle on/off making all tag levels mandatory.
3b.⚠️ theCustom tag namefield will be removed; users will be able to set theCustom tag namea level deeper for each of the tag levels (see 4b. below). - ✅ Clicking on a tag level name in the table will open the RHP, which will contain:
4a. ARequiredtoggle, when enabled it makes the tag level required coding on the workspace and setsRequiredin the mainTagspage table.
4b.⚠️ ACustom tag name> push input row, that opens a simple "Custom tag name" page with a text input to rename the tag level.
4c. A list of all of the tag values for that tag level (that opens up the individual tag value edit page with the same permissions and logic that exists today).
4d. checkboxes right-aligned each of the tag values to facilitate bulk select capabilities via the[Selected X v]button with the same permissions and logic that exists today.
✅ Additionally -> Tags that are not multi-tag are working just like before.
All the steps test well and the implementation works as expected.
Regarding the
Now regarding
- If we click on
Classesmain tag then click on any of theClassessub-tags -> we get "Page not found" in RHP. This does not happen with theCustomers/Projectsmain tag sub-tags.
Video - Issue 1
issue-1.mov
- If we click on
Classesmain tag, we can observe sub-tags with&symbol in their name which is not decoded into&(ampersand) and we're left with sub-tag labels likeR&Dinstead ofR&D. Additionally, if click on this we also get the RHP "Page not found" and the URL param looks weird as it's actually encoding the labelR&Dwhich is url encoded toR%26amp%3BDinstead of encoding decoded labelR&Dwhich should result in url encodedR%26D.
Video - Issue 2
issue-2.mov
❗ Now I'm not sure if these should be PR blockers or we're good to move forward despide these issues. If we're good, then we can move forward - otherwise these issues should be fixed before this is merged.
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #41279 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
| @@ -59,7 +59,7 @@ function QuickbooksImportPage({policy}: WithPolicyProps) { | |||
| }, | |||
| ]; | |||
|
|
|||
| if (policy?.connections?.quickbooksOnline.data.country !== CONST.COUNTRY.US) { | |||
| if (policy?.connections?.quickbooksOnline?.data?.country !== CONST.COUNTRY.US) { | |||
There was a problem hiding this comment.
Why is this change needed? TS does not report that these optional chaining operators are necessary.
There was a problem hiding this comment.
Yeah - problem here - that we have crash on this line - seem like TS types for data and country is not expecting to be undefined, but from API side it could :-)
just fyi
There was a problem hiding this comment.
thanks for the context! Let's update the TS types for data and country to be optional then? to make sure that we're safely accessing them everywhere, not just here.
There was a problem hiding this comment.
I have updated the type for data to be optional. We need to do the same for the Connection object: both quickbooksOnline and xero are optional. Tried to make that change here but caused ts errors on updatePolicyConnectionConfig. Let's create a separate issue to handle that
| }, [isFocused]); | ||
|
|
||
| const policyTagLists = useMemo(() => PolicyUtils.getTagLists(policyTags).filter((policy) => policy.name === currentTagListName), [currentTagListName, policyTags]); | ||
| const tagList = useMemo<PolicyForList[]>( |
There was a problem hiding this comment.
IMO this can be made much more readable, and also significantly reduce our type coercion using an implementation like this:
const tagList = useMemo<PolicyForList[]>(
() =>
policyTagLists
.map((policyTagList) => Object.values(policyTagList.tags))
.flat()
.sort((tagA, tagB) => localeCompare(tagA.name, tagB.name))
.map((tag) => ({
value: tag.name,
text: PolicyUtils.getCleanedTagName(tag.name),
keyForList: tag.name,
isSelected: selectedTags[tag.name],
pendingAction: tag.pendingAction,
errors: tag.errors ?? undefined,
enabled: tag.enabled,
isDisabled: tag.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
rightElement: <RightElementEnabledStatus enabled={tag.enabled} />,
})),
[policyTagLists, selectedTags],
);Also, I can't help but notice that this is exactly the same as the other file. So let's DRY this up by moving it to a util function.
There was a problem hiding this comment.
I didn't get the time to look into this one. I can handle later if we don't have to merge today otherwise it can be done in a follow up
There was a problem hiding this comment.
Some other high-level feedback on this PR is that it seems like there's a lot more room to optimize things:
- don't re-create an object that shadows an array on every render, use memoization and generate them at the same time
- don't copy-paste large segments of code. let's try to put in some work to do a reasonable amount of DRY
- question your type casts. Try removing them and see what happens. we should often be able to rely on type inference, and if need to cast types it's often a sign that something could be simplified. For example by using native array sorting rather than
lodashSortBy(which is slower and mangles types) - generally I try to avoid creating functions that just contain render logic and calling them on every render. Usually a better pattern is to store the JSX element(s) that we want to render in variables and use
useMemoto encapsulate the more complex render logic. It's the same level of work and readability, with better memoization often leading to better performance.
|
(btw @s77rt I know you took over this PR from @narefyev91, so hopefully this feedback is helpful to him as well) |
Yeah! But most of your comments related to already existed code :-) |
@MonilBhavsar @luacmartins I seem to remember you guys having some experience with this from categories. Context:
|
hayata-suenaga
changed the title
There was a problem hiding this comment.
Chatted about this in slack, but let's please make the two changes I requested here
|
I have pushed changes for the types. I didn't add a comment as it will only explain the what not the why. |
|
I was curious how this PR is going. I haven't checked the code but the UI looking really nice! Screen.Recording.2024-05-10.at.2.03.31.PM.mov |
hayata-suenaga
changed the title
|
✋ 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/roryabraham in version: 1.4.73-0 🚀
|
|
Why wasn't this PR tested on mobile? It created this deploy blocker here for example: #42059 |
|
Sorry my bad! I have raised a PR fixing the regression |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.73-7 🚀
|
Details
Fixed Issues
$ #41279
PROPOSAL:
Tests
ClassesandCustomers/ProjectsClassesandCustomers/ProjectsOffline tests
N/A
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop