-
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
Feat/categories main page #36324
Feat/categories main page #36324
Conversation
e2c3fec
to
db40fc3
Compare
@luacmartins, it appears that to access policy categories, we must first open a report, as I couldn't find any other API read command for fetching this data directly. Given that we're opening a workspace rather than a specific report, might we require an additional backend endpoint to retrieve this information? It would be great if we could include this data in the OpenWorkspace read command to streamline the process. |
16a09a4
to
3914f71
Compare
@luacmartins, regarding slack thread, I'm thinking about what we should do next because we're waiting for the OpenWorkspaceCategoriesPage API command. Can we keep going with other things, or should we wait and put this PR on hold? |
@ArekChr i think you can continue with as much as possible, as i explained everything works as expected in case of the primary workspace and that is a usecase for 90+% users they only have one workspace How is this blocking 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.
Sorry, I'm still ooo but I agree with @mountiny that we shouldn't hold on that
Asked in Slack here to confirm if this is ready for a C+ to conduct an initial review. |
I wasn't sure, thanks for the clear confirmation! Let's move forward |
b59fe2c
to
70d249f
Compare
@rushatgabhane is going to review this PR :) |
Maybe helpful for @rushatgabhane's reference, here is the categories page from the design doc on both web and mobile: |
@ArekChr @burczu I think we should use the same Stylized SelectionList wrapper - |
@ArekChr also, just to confirm... the |
I've observed that select all checkbox is too easy to hit, if you tap anywhere on the header, it selects all items. is it ok? Nagranie.z.ekranu.2024-02-23.o.11.09.17.mov |
yeah i think it's ok for now |
Oh hm, I don't think we want that long term though. Like the individual rows, it should be limited to the select all checkbox being clicked/pressed. |
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
🎯 @rushatgabhane, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #37135. |
@ArekChr could you add some test steps to the issue please? |
@luacmartins Updated tests steps |
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, we’ll have to address the checkbox toggling issue in a follow up, but we can tackle that when we update each row to navigate to the RHP.
✋ 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.44-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀
|
@@ -928,6 +929,7 @@ function getCheckboxPressableStyle(borderRadius = 6): ViewStyle { | |||
alignItems: 'center', | |||
// eslint-disable-next-line object-shorthand | |||
borderRadius: borderRadius, | |||
...cursor.cursorPointer, |
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.
return <FullscreenLoadingIndicator />; | ||
} | ||
|
||
if (shouldShowNotFoundPage) { |
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.
Not found page is shown in sidebar and main page causing two not found pages, ref: #39092
Details
This PR introduces a new categories page. This is a basic version, which means that the ability to select and unselect categories. This future update will allow for the displaying details, removal, and editing of selected categories.
Fixed Issues
$ 35705
PROPOSAL: https://docs.google.com/document/d/1gk3xqOs7epMbUrSSiX8K7YcqfPLVgqEos0sf-D-GMDA/edit#bookmark=id.qdiriu8utc5u
Tests
Offline tests
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop