Data Catalog Feature Component Breakout #946
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| description={getString('dataCatalogBanner').other} | ||
| /> | ||
| <LayoutProps title='Data Catalog' description={getString('dataCatalogBanner').other} /> | ||
| <PageHero title='Data Catalog' description={getString('dataCatalogBanner').other} /> |
There was a problem hiding this comment.
📓 : Moved layouts component from data-catalog into wrapper instead
| cardComponentOverride?: { // Allow user to override with a different cardType with optional props | ||
| CardComponent: ComponentType<CardComponentProps>; | ||
| props: Partial<CardComponentProps>; | ||
| } |
There was a problem hiding this comment.
📓 : Do we care to have the ability to override the card component? I've added this but i'm thinking we dont currently need it. If we do want to override card logic or styles, we can defer till we have that usecase and need to solve for. thoughts?
There was a problem hiding this comment.
I think we discussed this issue before, but to make it as a record: I don't think we need to offer ways of overriding specific components inside of the feature components (at lest for now.) If this is needed, a user can make their own list without overriding the specific components inside of the feature.
There was a problem hiding this comment.
I think this makes sense, if the user wants alot more flexibility or to use whatever card component they like, they will have the flexibility now to do this in their page views. i'll go ahead and remove
sandrahoang686
changed the title
sandrahoang686
requested review from
j08lue,
dzole0311,
hanbyul-here and
oliverroick
|
There's work planned to bring the data catalog layout to the layer selection modal on the E&A page.
That would be the part that would be used in the E&A modal, yes. The difference is that on the Data Catalog we're searching data collections and on the E&A page we're searching data layers within collections. This is the ticket for that 👉 #932 |
sandrahoang686
force-pushed
the
refactor/data-catalog-breakout
branch
from
bdd2fcc to
7e099b4
Compare
|
Note: @hanbyul-here @dzole0311 I created the |
|
Some of my thoughts on the bigger scheme of changes! Is the plan to put everything under the I thought the Data catalog component would export the whole page (without navigation - blue rectangle in the screenshot), but it will have three components inside that users can opt-in to use separately (red rectangles) if they don't need a whole page as it is. I am mainly thinking in this way for an easy integration of the existing instances later. 🤔 How do you think/
|
|
@hanbyul-here thanks reviewing and asking
No, I created the
With feature components, we should just isolate them to the actual product feature they are trying to deliver. So instead of thinking of entire pages to export and import, its more like we dont worry about layout and layout components like headers and footers. Those can be reuseable components we breakout though and the instance gets to dictate whether or not they'd want to use those layout components and the layout in general (for example, the reuseable headers we've broken out or just entirely different ones). This is where we give alot more control in terms of composability to the instance. No more override components and passing in JSX elements as props to the page. Instead the page is built on the instance side with layout and layout components controlled at that level. (we will still provide a set of reuseable layout components though like the existing header and footers and other reuseable non layout components like the datasets slider component) In regards to easy integration for existing instances later, It is still very easy, as you can see in the container component created. It easily recreates the page but gives so much more flexibility in terms of layout control. (this container element is supposed to replicate how it would look like on the instance side and was created here to just support our current instances until they are migrated over) Does this make sense? |
|
It makes total sense, @sandrahoang686 🙇 thanks for your reply. My apologies for revisiting some items that we already discussed, such as layouts from VEDA-UI. My thoughts go back and forth as I start working on the test instance. Thanks for pointing out the container component. I think the migration should be okay as long as we export the existing layout components from VEDA-UI. I am still a bit concerned that the global style from the instances interrupts how the components can be composited—for example, can our catalog-view component accommodate the dark mode set by the instance? But the way that I suggested won't necessarily resolve this problem now I think.
If this component can be reused inside of Veda-ui, how about moving it to |
|
@hanbyul-here as per our discussion, i've moved the catalog view under common as |
There was a problem hiding this comment.
The last changes and the discussion points make sense to me 👍 Aside from two nits, the rest lgtm, thanks @sandrahoang686
| @@ -1,7 +1,7 @@ | |||
| import React from 'react'; | |||
| import { getString } from 'veda'; | |||
| import { allDatasets } from '$components/exploration/data-utils'; | |||
There was a problem hiding this comment.
Nit: To avoid coupling features, should we create a common/data-utils.ts to move the allDatasets util there and use it in both, the exploration and the data-catalog?
## What's Changed We are bumping the major version up since it won't be compatible with the previous version of UI with the bumped up Node version ## 🎉 Features - Update Node Version to v20 in #917 , #971 ## 🚀 Improvements - Replace old map component with new map component in Story Maps in #827 - Data Catalog Feature Component Breakout in #946 - Add layer description to layer modal in #955 - ## 🐛 Fixes 🦗
**Related Ticket:** Very vaguely related to #946 This PR makes the Card component delegate what link component to render to the `SmartLink` component. I initially started working on this PR to continue the work on #1096, more specifically, to make the link component come from props. I thought this change was worth merging regardless of whether we work on the routing problem now or later.
Related Ticket: #892
Data Catalog now takes in datasets as a prop and to support the current architecture, a wrapper container was created to interact with the data-layer to provide data to this feature component (PR#893). Note: The template instance wouldn't need this wrapper container as they would have their own page views that would interact with the data layer. Page layout / styling is also removed from Data-catalog component into the wrapper (PR#930).
@j08lue @aboydnw @faustoperez The data-catalog feature component is now just what is highlighted in the red box. So when this feature is imported, they would only get what i've highlighted here. Page layout and headers would be something the actual page manages now. Does this seem fine to you both?
Description of Changes
Notes & Questions About Changes
{Add additonal notes and outstanding questions here related to changes in this pull request}
Validation / Testing
{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}