-
Notifications
You must be signed in to change notification settings - Fork 18
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
95 - Invoke js dataverse use cases in the dataset page #112
95 - Invoke js dataverse use cases in the dataset page #112
Conversation
…into feature/95-invoke-js-dataverse-use-cases-in-the-dataset-page
…-client-javascript npm package from GitHub registry
…hub.com/IQSS/dataverse-frontend into feature/95-invoke-js-dataverse-use-cases-in-the-dataset-page
…-page' of https://github.com/IQSS/dataverse-frontend into feature/95-invoke-js-dataverse-use-cases-in-the-dataset-page
Just for the record, the token thing worked for me. Happy to help troubleshoot. |
Mmmm, that's strange. We can take a look at it together if you're available today |
In the suggestions for testing
It should be username: dataverseAdmin |
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.
This looks great! I ran the tests in the instructions in the local environment. I made some comments about the build.
The only issue I found is that when you search for a version that doesn't exist, it displays the latest version, but doesn't have the "version not found" info box. I think this is a small thing, so we could track it in a separate issue, if it is a big fix.
![Screenshot 2023-05-31 at 1 19 16 PM](https://private-user-images.githubusercontent.com/675224/242356207-da7b02dc-ac65-4e21-ae46-abd8d9e3e852.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2OTg3MjQsIm5iZiI6MTczOTY5ODQyNCwicGF0aCI6Ii82NzUyMjQvMjQyMzU2MjA3LWRhN2IwMmRjLWFjNjUtNGUyMS1hZTQ2LWFiZDhkOWUzZTg1Mi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE2JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxNlQwOTMzNDRaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT04YjczZjdmOTRhYTIwNzIwY2IxOWQxZThmNDY3MGRmZmYxZjVlMmZiYjNiYmRkODhiMzY1YmMwMWRjOWE4MDFjJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.CC2WCPVyEgmDMkQvhiR1E5osFkSKCvzTd901UzUlXRk)
packages/design-system/package.json
Outdated
"@types/react": "^18.0.27", | ||
"typescript": "^4.9.5", | ||
"vite-plugin-istanbul": "^4.0.1", | ||
"bootstrap": "^5.2.3", |
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.
I got an error building locally, so I changed bootstrap version to "5.2.23"
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.
Thanks for testing it meticulously! I was thinking of assessing all the messages in this issue
Because there are more info messages like this one and I was thinking of creating a react context or something like that where we can throw any message at any point of the application and then the context updates the alert banner. We can keep track of the existing messages by adding comments on that issue so we don't miss any message. What do you think?
I mean, if this info box is important for the demo I can add it now, but I'd prefer to asses all the messages at once in a separate issue
fixed, thanks for your help :) |
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.
Just a quick comment on the need for this change in packages/design-system/package.json:
- "bootstrap": "^5.2.3",
+ "bootstrap": "5.2.3",
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.
I've only tested "view mode" but this is looking great!
The one thing I noticed from a minimal example (screenshot below )is that the ORCID ID is not a link. The JSF xhtml was edited to support this in IQSS/dataverse#7979 but I'm in favor of merging this and working on links like this in the future.
While I'm staring at this... some comments on the dataset summary. I don't see these mentioned in the PR where it was added:
I think that's it. Not sure how we're tracking this stuff. 🤔 Update: I created this issue: |
I noticed this too and went ahead and fixed it. |
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
…into feature/95-invoke-js-dataverse-use-cases-in-the-dataset-page
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.
I updated the Chromatic build to accept the visual changes. Approved!
b2e7ac2
to
513959f
Compare
…use-cases-in-the-dataset-page 95 - Invoke js dataverse use cases in the dataset page
What this PR does / why we need it:
This PR adds the use cases of the js-dataverse module to the Dataset page. This means that we're now able to show real datasets in the frontend.
This PR also improves the containerized development environment
Which issue(s) this PR closes:
Special notes for your reviewer:
Changes dev-env
Changes datasets UI
Suggestions on how to test this:
Step 1: Get a personal token to use the js-dataverse registry
.npmrc.example
file without the .example suffixnpm i
Step 2: Run the application locally
npm i
cd packages/design-system && npm run build
.env
file such as the .env.example, with theVITE_DATAVERSE_BACKEND_URL=http://localhost:8000
variablenpm start
Step 3: Run the development environment
sudo chmod +x /usr/bin/jq
. On Mac, you can install it withbrew install jq
if you use homebrew: http://brew.sh/ .cd dev-env
./run-env.sh 9588-datasets-api-extension
If it gives a timeout before the containers are ready, run again the above command
Step 4: Test Dataset View mode
?persistentId=doi:10.5072/FK2/LHGRHP&version=DRAFT
Step 4: Test an specific dataset version
Step 5: Test a dataset using a privateUrlToken
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?:
Display a dataset by persistentId or privateUrlToken in the SPA
Improve the development environment of the SPA
Additional documentation: