230 - Add dataset card info to Home page #248
Conversation
MellyGray
added
the
UI
| @@ -53,7 +53,13 @@ export function DatasetCitation({ thumbnail, title, citation, version }: Dataset | |||
| ) | |||
| } | |||
|
|
|||
| function CitationDescription({ citation, version }: { citation: string; version: DatasetVersion }) { | |||
| export function CitationDescription({ | |||
There was a problem hiding this comment.
Since this is also being used in the DatasetCard, would it make sense to move it to a shared folder?
| isDeaccessioned?: boolean | ||
| } | ||
|
|
||
| export function DatasetThumbnail({ thumbnail, title, isDeaccessioned }: DatasetThumbnailProps) { |
There was a problem hiding this comment.
Same comment as CitationDescription, should this be moved to a shared folder?
There was a problem hiding this comment.
I also had that debate with myself
The way I see it, the/sharedfolder is not intended for components shared between pages but rather for components that are general enough not to belong to a specific domain entity.
As a result, I refactored CitationDescription to remove the datasetVersion prop, making it more generic, and then moved the CitationDescription component to the /shared folder.
In the case of the DatasetThumbnail, it's tightly coupled to the Dataset domain, so I believe it belongs in the dataset section. I don't see any issues with sharing it with DatasetCard
Let me know what you think
There was a problem hiding this comment.
Ok, I can definitely see both sides of that too. The way I understand the src/sections/dataset folder, is that it contains all the components specific to the view dataset page, as opposed to the dataset domain, but they are similar concepts. I think it's ok to keep DatasetThumbnail where it is.
| persistentId: faker.datatype.uuid(), | ||
| title: faker.lorem.sentence(), | ||
| labels: DatasetLabelsMother.create(), | ||
| isDeaccessioned: false, |
There was a problem hiding this comment.
It might be good to change isDeaccessioned from false to faker.datatype.boolean(), to get some deaccessioned datasets in the test data
There was a problem hiding this comment.
You're right, it wasn't that straightforward but I did the changes to show some deaccessioned datasets in the storybook
There was a problem hiding this comment.
I noticed that the JSF version of the card has a small upper right Dataset Icon (to distinguish it from Dataverse cards that also appear in the list.). Is that something we are skipping for now?
There was a problem hiding this comment.
I added the icon, thanks!
|
Looks good. Merging to test the complete feature on feature/229-basic-home-page-with-the-list-of-datasets |
GPortas
merged commit ff238ce
into
feature/229-basic-home-page-with-the-list-of-datasets
cmbz
added
pm.GREI-d-2.7.1
…-home-page 230 - Add dataset card info to Home page
What this PR does / why we need it:
This PR adds the dataset card information to the list of datasets in the Home Page.
Which issue(s) this PR closes:
Special notes for your reviewer:
Please, check that the Home Page PR is reviewed before this one:
Also note that the date format on the card will depend on your browser's language
Suggestions on how to test this:
npm install.cd packages/design-system && npm run build.cd ../../.npm run storybook.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Mock of the dataset info
Final result
Is there a release notes update needed for this change?:
Additional documentation: