-
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
Feature/188 dataset download options #232
Conversation
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.
Take a look at the comments and let me know if you have any questions!
src/sections/dataset/dataset-action-buttons/DatasetActionButtons.tsx
Outdated
Show resolved
Hide resolved
src/sections/dataset/dataset-action-buttons/access-dataset-menu/AccessDatasetMenu.tsx
Outdated
Show resolved
Hide resolved
src/sections/dataset/dataset-action-buttons/access-dataset-menu/AccessDatasetMenu.tsx
Outdated
Show resolved
Hide resolved
src/sections/dataset/dataset-action-buttons/access-dataset-menu/AccessDatasetMenu.tsx
Outdated
Show resolved
Hide resolved
...onent/sections/dataset/dataset-action-buttons/access-dataset-menu/AccessDatasetMenu.spec.tsx
Outdated
Show resolved
Hide resolved
Thanks Melina! I made the changes you suggested. |
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.
Looks good! Thanks for applying the suggestions
@ekraffmiller Please, can you resolve the conflicts? |
@ekraffmiller For stories in the AccessDatasetMenu section in Storybook, I see that there is an extra parenthesis. For stories directly under DatasetActionButtons folder, in addition to the extra parenthesis I find empty values. Only With Publish Permissions has values. I'm not sure if this is correct, but it looks a little confusing: |
Thanks @GPortas, I made some fixes, ready for QA again |
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.
ugh, sorry I missed that one! Should be fixed now. |
removed extra parenthesis |
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!
…tions Feature/188 dataset download options
What this PR does / why we need it:
Implements the download options for the AccessDatasetMenu
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Review the Stories in AccessDatasetMenu
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?:
Additional documentation: