-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(ui): Show all views in settings #14971
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
base: master
Are you sure you want to change the base?
Conversation
Bundle ReportChanges will increase total bundle size by 5.4kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
nice! overall looking solid. two minor comments then will be ready to merge i think
const currentPath = window.location.pathname; | ||
|
||
const currentTab = Object.entries(tabUrlMap).find(([, url]) => url === currentPath)?.[0] as TabType; | ||
if (currentTab) { | ||
setSelectedTab(currentTab); | ||
} else { | ||
setSelectedTab(null); | ||
} | ||
} | ||
}, [selectedTab]); | ||
|
||
const getCurrentUrl = useCallback(() => window.location.pathname, []); |
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.
let's always try to use location
from useLocation
instead of the window object directly - it's generally safer and factors in stuff like if we have a basePath prefixed to our site. which we now support :)
? (data as ListMyViewsQuery)?.listMyViews | ||
: (data as ListGlobalViewsQuery)?.listGlobalViews; |
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 are trying to set a hard constraint against type casting. is it possible to do this without casting and in a type safe way?
you could have both queries and skip
if you're not on the correct view. then get data from one or the other based on isPersonal
Currently, the only way to see all the views in the instance is via the Views bar next to the Search. In the Manage Views page the users can only see the views created by themselves and not the public views.
This would be confusing to users (especially admins and other with manage_global_views permission) who'd want to manage all the views available in the instance.
Adding this new tab under views page can resolve that confusion. If they have the permissions they can edit these public views, otherwise they can just preview them.
Before:

After:


Notes: