Skip to content
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

Requirements April 2024 (JS) #8157

Merged
merged 17 commits into from
Apr 25, 2024
Merged

Requirements April 2024 (JS) #8157

merged 17 commits into from
Apr 25, 2024

Conversation

amercader
Copy link
Member

@amercader amercader commented Apr 5, 2024

Follow up from #8146, that should be merged first

This upgrades the frontend requirements to the latest version.

Several issues found:

I reverted the upgrade from Bootstrap 5.1 to 5.3 because there were a lot of visual changes and minor breakages that would be better fixed in a separate PR, possibly as part of the frontend refresh that @thegostev is leading:

Before (current)

Screenshot 2024-04-08 at 14-52-46 Dataset - CKAN

After

Screenshot 2024-04-08 at 14-44-14 Dataset - CKAN

Moved the cleanup code to a fixture
The new Cypress version renames a lot of files but the suite itself
hasn't been changed
Remove completely our own version parsing and rely on the one from
packaging. Older versions of packaging used to be more lenient with our
release tags ("ckan-x.y.z") but now they will fail. We need to remove
the `ckan-` bit before parsing.
When upgrading cypress to >= 12.0.0 many of our tests started failing.
That's because starting from Cypress 12.0.0 the `testIsolation` option
is enabled by default:

https://docs.cypress.io/guides/references/migration-guide#Test-Isolation

As our old Pylons server side tests, it looks like the frontend ones
were written in a way where tests are not isolated and rely
on previous state. To get the tests working again we are passing
`{testIsolation: false}` to each test spec, but as stated in the docs
above this is not recommended and we should rewrite our tests so they
can be run independently.
The updated accessibility tests were flagging issues with the `link-in-text-block`
check, that ensures that links can be suficiently distinct from
surrounding text. Rather than changing the color, this adds an underline
to links **within paragraphs**, following the available guidelines:

https://accessibilityinsights.io/info-examples/web/needs-review/link-in-text-block/
@amercader
Copy link
Member Author

One thing the updated accessibility tests flagged was issues with the link-in-text-block check, that ensures that links are sufficiently distinct from surrounding text. Rather than changing the color, I opted for adding an underline to links within paragraphs, following the available guidelines:

https://accessibilityinsights.io/info-examples/web/needs-review/link-in-text-block/

Before:
Screenshot 2024-04-08 at 16-04-17 About - CKAN

After:

Screenshot 2024-04-08 at 15-42-31 About - CKAN

If accepted, this should be flagged in the changelog

@amercader
Copy link
Member Author

@smotornyuk One thing I'm confused about are the typing errors. I've updated pyright to 1.1.357 but quickly reverted it as there were about 90 new failures (that will be better fixed separately). But it seems that even sticking with the previous version I still get some new failures. Any idea?

@amercader amercader marked this pull request as ready for review April 8, 2024 14:07
@smotornyuk
Copy link
Member

Found a solution - run npm i 'pyright@1.1.339' and commit changes to package.json and package-lock.json

NPM's dependency resolver incorrectly resolved versions of packages used by pyright and cypres and, as result, pyright was downgraded to an older version. npm i 'pyright@1.1.339' guarantees that NPM uses this exact version of pyright(that's the version used in the master branch)

@amercader amercader added this to the CKAN 2.11 milestone Apr 23, 2024
@amercader
Copy link
Member Author

@smotornyuk thanks, done now. Typing error unrelated, see #8196 (comment)

@smotornyuk smotornyuk merged commit 5baee81 into master Apr 25, 2024
6 of 8 checks passed
@smotornyuk smotornyuk deleted the requirements-april-2024-js branch April 25, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants