-
Notifications
You must be signed in to change notification settings - Fork 16
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
129 - Dataset Action Buttons UI #181
Conversation
…into feature/129-dataset-page-action-buttons
…into feature/129-dataset-page-action-buttons
…into feature/129-dataset-page-action-buttons
…into feature/129-dataset-page-action-buttons
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.
@@ -234,13 +262,40 @@ export class Dataset { | |||
public readonly labels: DatasetLabel[], | |||
public readonly summaryFields: DatasetMetadataBlock[], | |||
public readonly license: DatasetLicense, | |||
public readonly metadataBlocks: DatasetMetadataBlocks | |||
public readonly metadataBlocks: DatasetMetadataBlocks, | |||
public readonly permissions: DatasetPermissions, |
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.
So to create an instance of a dataset model, you will also need to obtain the permissions that the current user has on the particular dataset. I'm not sure if coupling these permissions within the dataset model can be problematic.
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.
Exactly like that, in order to create the Dataset you'll need the user permissions.
I understand your concern and these are the reasons to have them together:
- The dataset permissions are always used with the dataset model. So I think these permissions belong to the Dataset model, for keeping the cohesion
- If we have another page where we want to use the dataset permissions without the model. We can do it, getDatasetPermissions and you return the DatasetPermissions interface. But this is not going to happen in the Dataset Page, in the dataset page we always want the dataset and the dataset permissions, so it makes sense to have them together.
- I separated the File Permissions from the File model and it's being a nightmare, you need to remember to call these permissions when you get the files, initialise the React context, wait for them to be fetched, the tests are much more complex. Too much complexity for the aim of keeping the separation of concerns, when in reality I never use the file permissions without the files itself. I want to refactor that but I was also waiting for some feedback on this other approach.
I think the Dataset model is in reality the DatasetPage model, so its aim is to represent a DatasetPage, and the dataset page doesn't make sense without the permissions.
Very interesting conversation, please let me know if you can think of some problems of this approach
@GPortas I applied the requested changes
|
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!
…-buttons 129 - Dataset Action Buttons UI
What this PR does / why we need it:
This PR adds the Dataset Action Buttons UI to the Dataset Page View Mode
Which issue(s) this PR closes:
Special notes for your reviewer:
Some dataset properties are not connected to js-dataverse yet, so take that into account that if you test the e2e. For example the LinkDatasetButton is always going to appear in the e2e page, but if you look at the unit tests the LinkDatasetButton will only appear if the Dataset is released.
Changes in this PR
Suggestions on how to test this:
To test the changes introduced by this PR, follow these steps:
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:
Is there a release notes update needed for this change?:
Dataset Action Buttons added to the Dataset Page
Additional documentation: