-
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
78 - Create the boilerplate of the dataset page #96
78 - Create the boilerplate of the dataset page #96
Conversation
…into feature/78-create-the-boilerplate-of-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.
@MellyGray I made an initial pass of changed files. It feels like we're losing some things such as layers and the recent addition of the containerized dev environment. Can you please take a look? Thanks.
.env
Outdated
@@ -1 +1 @@ | |||
VITE_DATAVERSE_BACKEND_URL=http://localhost:8080 | |||
VITE_DATAVERSE_BACKEND_URL=http://http://localhost:8000 |
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.
Is this right? Double http?
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.
You're right, that's wrong. I was also thinking, should we add the .env to the .gitignore?
dev-env/docker-compose-dev.yml
Outdated
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.
It doesn't seem right to delete this docker compose file. It was just added by @GPortas in in this PR:
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.
Wow, my bad, restored!
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.
What is the purpose of this file?
Should we git rm
it?
And then add it to .gitignore?
I'm asking because I see similar entries there:
$ ack .nyc_output
.gitignore
15:/.nyc_output
19:.nyc_output
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.
Yes, I added packages/design-system/.nyc_output
to the gitignore, thanks!
.primary { | ||
color: color-contrast($dv-primary-color); | ||
} | ||
.primary:not(#\#) { |
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.
No more layers? Are we removing layers?
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 tried to explain it in the PR description
I had to remove CSS cascade layers from de dataverse-design-system because Vite does not support injecting styles in Lib Mode, vitejs/vite#1579 . Since Vite does not support this I had to use a plugin vite-plugin-libcss which does not support cascade layers. We can still use cascade layers in the main app.
Cascade layers are not supported by the preprocessor for the Lib Mode of the design system
But we can still use cascade layers in the main application
We can create an issue to try to do some workaround for this
@@ -0,0 +1,4 @@ | |||
{ | |||
"heading": "Page Not Found", | |||
"message": "The page you are looking for was not found. If you believe this is an error, please contact Demo Dataverse Support for assistance." |
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 is fine for now, I suppose, but in the future, instead of "Demo Dataverse Support" as a hard-coded string, we'll want to pull this from the backend.
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.
Yes, I saw there is a Modal to contact Dataverse Support, we can create a new issue to add this feature
host: true, | ||
port: 5173, | ||
hmr: { | ||
clientPort: 8000 // nginx reverse proxy port |
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'm pretty sure this nginx stuff is also something that was added in the recently "dev env" PR.
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.
Yes, this code was added for the dev-env to work, but it's breaking the npm start
command. You can test it in the develop branch if you run npm start
and you go to the browser
So I asked Guillermo about this and he said he'll work on fixing it, but for the moment this was a workaround to be able to use npm start
Should I comment the code instead of removing it?
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.
If we are basically reverting this PR about adding a docker compose dev environment...
... @MellyGray can you please add this under "Special notes for your reviewer"?
@GPortas should we re-open the following issue?
Or perhaps create a new issue and put it on the schedule?
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 just asked about this during a frontend meeting and new commits have been added to bring back the docker compose file. I'll take a look!
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.
The docker-compose is back, but the nginx reverse proxy port I had to remove it to fix the npm start
. So we still need to manage that with a new issue
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.
@GPortas can you please create an issue for nginx? ^^
Currently, the README says Storybook is on port 6006...
... but these days it seems like Storybook is on both 6006 and 6007. I was confused because I didn't realize at first that two browser are being opened. I only saw 6007 (which opened second, at least on my machine) and was wondering where the pages and layouts are. 🤔 Here the are! On the other port. Can we please update the README about this? I can only assume this is all by design. (I'm not sure if this behavior is new as of this PR or not.) |
Thanks! I updated the README. We have two instances of Storybook one for the Design System and one for the application. I'm still trying to figure out the development workflow so for the moment I think two instances are helpful, but we'll see. We could be moving everything to 1 Storybook in the future if this configuration doesn't help the development workflow |
I added a change in the Style Guide to the README |
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'm having trouble running npm run test:e2e
.
@@ -0,0 +1,17 @@ | |||
describe('Dataset', () => { | |||
it('successfully loads a dataset when passing the id', () => { | |||
cy.visit('/dataset/12345') |
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'm getting a failure when trying to reach this URL. Should it have /spa in it?
Running: sections/dataset/Dataset.spec.tsx (2 of 4)
Dataset
1) successfully loads a dataset when passing the id
2) renders Hello Dataverse when no id is provided
0 passing (190ms)
2 failing
1) Dataset
successfully loads a dataset when passing the id:
CypressError: `cy.visit()` failed trying to load:
http://localhost:5173/dataset/12345
The response we received from your web server was:
> 404: Not Found
This was considered a failure because the status code was not `2xx`.
If you do not want status codes to cause failures pass the option: `failOnStatusCode: false`
at <unknown> (http://localhost:5173/__cypress/runner/cypress_runner.js:147661:84)
at visitFailedByErr (http://localhost:5173/__cypress/runner/cypress_runner.js:147069:12)
at <unknown> (http://localhost:5173/__cypress/runner/cypress_runner.js:147644:13)
at tryCatcher (http://localhost:5173/__cypress/runner/cypress_runner.js:18744:23)
at Promise._settlePromiseFromHandler (http://localhost:5173/__cypress/runner/cypress_runner.js:16679:31)
at Promise._settlePromise (http://localhost:5173/__cypress/runner/cypress_runner.js:16736:18)
at Promise._settlePromise0 (http://localhost:5173/__cypress/runner/cypress_runner.js:16781:10)
at Promise._settlePromises (http://localhost:5173/__cypress/runner/cypress_runner.js:16857:18)
at _drainQueueStep (http://localhost:5173/__cypress/runner/cypress_runner.js:13451:12)
at _drainQueue (http://localhost:5173/__cypress/runner/cypress_runner.js:13444:9)
at ../../node_modules/bluebird/js/release/async.js.Async._drainQueues (http://localhost:5173/__cypress/runner/cypress_runner.js:13460:5)
at Async.drainQueues (http://localhost:5173/__cypress/runner/cypress_runner.js:13330:14)
From Your Spec Code:
at Context.eval (http://localhost:5173/__cypress/tests?p=tests/e2e/sections/dataset/Dataset.spec.tsx:7:10)
2) Dataset
renders Hello Dataverse when no id is provided:
CypressError: `cy.visit()` failed trying to load:
http://localhost:5173/dataset
The response we received from your web server was:
> 404: Not Found
This was considered a failure because the status code was not `2xx`.
If you do not want status codes to cause failures pass the option: `failOnStatusCode: false`
at <unknown> (http://localhost:5173/__cypress/runner/cypress_runner.js:147661:84)
at visitFailedByErr (http://localhost:5173/__cypress/runner/cypress_runner.js:147069:12)
at <unknown> (http://localhost:5173/__cypress/runner/cypress_runner.js:147644:13)
at tryCatcher (http://localhost:5173/__cypress/runner/cypress_runner.js:18744:23)
at Promise._settlePromiseFromHandler (http://localhost:5173/__cypress/runner/cypress_runner.js:16679:31)
at Promise._settlePromise (http://localhost:5173/__cypress/runner/cypress_runner.js:16736:18)
at Promise._settlePromise0 (http://localhost:5173/__cypress/runner/cypress_runner.js:16781:10)
at Promise._settlePromises (http://localhost:5173/__cypress/runner/cypress_runner.js:16857:18)
at _drainQueueStep (http://localhost:5173/__cypress/runner/cypress_runner.js:13451:12)
at _drainQueue (http://localhost:5173/__cypress/runner/cypress_runner.js:13444:9)
at ../../node_modules/bluebird/js/release/async.js.Async._drainQueues (http://localhost:5173/__cypress/runner/cypress_runner.js:13460:5)
at Async.drainQueues (http://localhost:5173/__cypress/runner/cypress_runner.js:13330:14)
From Your Spec Code:
at Context.eval (http://localhost:5173/__cypress/tests?p=tests/e2e/sections/dataset/Dataset.spec.tsx:13:10)
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.
Yes, I updated the Cypress configuration to run on /spa. Thanks!
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'm not sure where to report this but I'm having trouble running the e2e tests. It's complaining that I'm missing the chai
dependency:
Running: info/infrastructure/repositories/DataverseInfoJSDataverseRepository. (3 of 4)
spec.ts
[vite]: Rollup failed to resolve import "chai" from "/Users/pdurbin/github/iqss/dataverse-frontend/tests/e2e/info/infrastructure/repositories/DataverseInfoJSDataverseRepository.spec.ts".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
Oops...we found an error preparing this test file:
> tests/e2e/info/infrastructure/repositories/DataverseInfoJSDataverseRepository.spec.ts
The error was:
Error: [vite]: Rollup failed to resolve import "chai" from "/Users/pdurbin/github/iqss/dataverse-frontend/tests/e2e/info/infrastructure/repositories/DataverseInfoJSDataverseRepository.spec.ts".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
at onRollupWarning (file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/vite/dist/node/chunks/dep-b6b8dfbf.js:44766:19)
at onwarn (file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/vite/dist/node/chunks/dep-b6b8dfbf.js:44536:13)
at Object.onwarn (file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/rollup/dist/es/shared/node-entry.js:25305:13)
at ModuleLoader.handleInvalidResolvedId (file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/rollup/dist/es/shared/node-entry.js:23940:26)
at file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/rollup/dist/es/shared/node-entry.js:23900:26
at processTicksAndRejections (node:internal/process/task_queues:95:5)
This occurred while Cypress was compiling and bundling your test code. This is usually caused by:
- A missing file or dependency
- A syntax error in the file or one of its dependencies
Fix the error in your code and re-run your tests.
(Results)
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 0 │
│ Passing: 0 │
│ Failing: 1 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: false │
│ Duration: 0 seconds │
│ Spec Ran: info/infrastructure/repositories/DataverseInfoJSDataverseRepository.spec.ts │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
────────────────────────────────────────────────────────────────────────────────────────────────────
Running: users/infrastructure/repositories/UserJSDataverseRepository.spec.ts (4 of 4)
[vite]: Rollup failed to resolve import "chai" from "/Users/pdurbin/github/iqss/dataverse-frontend/tests/e2e/users/infrastructure/repositories/UserJSDataverseRepository.spec.ts".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
Oops...we found an error preparing this test file:
> tests/e2e/users/infrastructure/repositories/UserJSDataverseRepository.spec.ts
The error was:
Error: [vite]: Rollup failed to resolve import "chai" from "/Users/pdurbin/github/iqss/dataverse-frontend/tests/e2e/users/infrastructure/repositories/UserJSDataverseRepository.spec.ts".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
at onRollupWarning (file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/vite/dist/node/chunks/dep-b6b8dfbf.js:44766:19)
at onwarn (file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/vite/dist/node/chunks/dep-b6b8dfbf.js:44536:13)
at Object.onwarn (file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/rollup/dist/es/shared/node-entry.js:25305:13)
at ModuleLoader.handleInvalidResolvedId (file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/rollup/dist/es/shared/node-entry.js:23940:26)
at file:///Users/pdurbin/github/iqss/dataverse-frontend/node_modules/rollup/dist/es/shared/node-entry.js:23900:26
at processTicksAndRejections (node:internal/process/task_queues:95:5)
This occurred while Cypress was compiling and bundling your test code. This is usually caused by:
- A missing file or dependency
- A syntax error in the file or one of its dependencies
Fix the error in your code and re-run your tests.
(Results)
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 0 │
│ Passing: 0 │
│ Failing: 1 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: false │
│ Duration: 0 seconds │
│ Spec Ran: users/infrastructure/repositories/UserJSDataverseRepository.spec.ts │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
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.
The chai module was missing in the package.json, maybe because of some merge conflict. Thanks! You'll need to run npm install
again to install the chai module
I made a few tweaks to the code: 35d7177 fix test:unit command http://localhost:5173/spa/datasets/12345 looks fine. Here's how it looks for me (I made it "Dataset Title" to be a bit more realistic): I'm basically ready to approve this accept that I can't get the e2e tests running. Please see #96 (review) |
Thanks! I fixed the e2e tests for the Dataset page, although we don't have much to test since the js-dataverse module hasn't implemented the dataset endpoint yet |
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.
Overall, this looks fine to me so I'm moving it to QA.
However, please note that e2e tests are known to be failing:
"Yes, the UserJSDataverseRepository e2e tests are not working yet, but this comes from the login logout branch, is not because of the Dataset changes.
Since we need to log in in JSF and use that cookie in the SPA, we still do not have those tests working"
-- @MellyGray at https://iqss.slack.com/archives/C04MJ76A34Z/p1684252920863049?thread_ts=1684246047.311379&cid=C04MJ76A34Z
I assume we'll fix these in a future PR.
I might need some help setting this up (Check that you have .env with a backend url). For QA, would this be using the deploy to payara action, making sure I have develop Dataverse already deployed to the same box?
|
The If you use the payara deployment then you don't need to set the .env, the .env is just for running the application locally using
Sorry! I forgot that step |
@MellyGray Thanks, that works now. I am seeing two e2e tests failing. Is this expected at this time? 0 passing (309ms)
Otherwise, this looks good and I can merge it. OK, I see from Phil's comment about and link to Slack conversation it is expected. So, merging! |
…e-of-the-dataset-page 78 - Create the boilerplate of the dataset page
What this PR does / why we need it:
This PR is the spike issue for the frontend work for this feature
Here we add the Dataset page.
Features:
Route:
datasets/{id}
.States:
Which issue(s) this PR closes:
Special notes for your reviewer:
vite.config.ts
to be able to run thenpm start
command. So we'll need to open a new issue to restore the proxy without breaking the command.Suggestions on how to test this:
Application
cd back to the root.
Check that you have a
.env
file such as the .env.example, with theVITE_DATAVERSE_BACKEND_URL
variable pointing to you local dataverse installation or if you don't have Dataverse running locally, you can point to https://demo.dataverse.org although some functionalities may not workGo to http://localhost:5173/spa/datasets/12345
You will see the Datasets page with a mocked api call
Storybook
To see all the different states:
Go to the Dataset section to see the different states of the page
Tests
npm run test:unit
or
npm run start
npm run test:e2e
Just remember that the e2e tests are not finished because the js-dataverse module is not ready
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 Page added to the application.
Additional documentation: