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

Concept page template and Cypress tests #1549

Merged
merged 21 commits into from
Nov 22, 2023

Conversation

osma
Copy link
Member

@osma osma commented Nov 14, 2023

Reasons for creating this PR

Reimplement the concept page template so it follows the new Skosmos 3 design.
Also add Cypress tests verifying the existing functionality.

This implements just the basic functionality. Many features and special cases are not implemented yet; see checklist in #1484 (comment)

Link to relevant issue(s), if any

Description of the changes in this PR

  • refactor Vocabulary.getConceptInfo() so it returns a single concept (instead of an array of size 1)
  • rewrite the concept page template based on the static example (but adapting it heavily to use Bootstrap Grid instead of a HTML table)
  • add some Cypress tests for the concept page / template

Known problems or uncertainties in this PR

n/a

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma added this to the 3.0 milestone Nov 14, 2023
@osma osma self-assigned this Nov 14, 2023
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (b4e41b4) 70.51% compared to head (cff4180) 70.53%.
Report is 2 commits behind head on skosmos-3.

Files Patch % Lines
src/controller/RestController.php 0.00% 2 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##             skosmos-3    #1549      +/-   ##
===============================================
+ Coverage        70.51%   70.53%   +0.01%     
  Complexity        1643     1643              
===============================================
  Files               32       32              
  Lines             4314     4313       -1     
===============================================
  Hits              3042     3042              
+ Misses            1272     1271       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@osma osma force-pushed the issue1484-concept-page-templates-cypress branch from 64d0f24 to 4293474 Compare November 15, 2023 10:34
@osma osma force-pushed the issue1484-concept-page-templates-cypress branch from 09ef9cf to ed7db46 Compare November 16, 2023 14:34
@osma osma marked this pull request as ready for review November 16, 2023 14:54
@osma osma requested a review from joelit November 16, 2023 14:54
@osma osma changed the title WIP: Concept page template and Cypress tests Concept page template and Cypress tests Nov 16, 2023
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 🥇

@osma
Copy link
Member Author

osma commented Nov 17, 2023

I see that there's a little regression on the vocabulary home page, for example some background colors have changed. This is most likely due to shared CSS rules. I will try to fix that before merging.

Copy link
Contributor

@joelit joelit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a couple of suggestions for this PR. I also wrote further requirements for concept pages in the comments for the associated issue #1484 that would be out of scope for this PR.

his is a solid start! Thanks especially for dismantling some of the old Skosmos 2.x template logic that is no longer in use. The cypress tests are also a sight worth seeing. Two michelin stars!

resource/css/skosmos.css Show resolved Hide resolved
src/view/concept-card.inc Outdated Show resolved Hide resolved
tests/cypress/template/concept.cy.js Show resolved Hide resolved
@osma
Copy link
Member Author

osma commented Nov 21, 2023

Still TODO:

  • fix headline styling regressions on feedback and about pages
  • fix background color regressions on vocab-home page

Copy link
Contributor

@joelit joelit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@osma
Copy link
Member Author

osma commented Nov 22, 2023

Hmm, something broke in the Cypress tests, need to fix that as well before merging

Copy link
Contributor

@joelit joelit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be problems with two cypress tests that should be fixed

tests/cypress/template/concept.cy.js Show resolved Hide resolved
tests/cypress/template/concept.cy.js Show resolved Hide resolved
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

No Coverage information No Coverage information
26.5% 26.5% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@osma osma merged commit b54607b into skosmos-3 Nov 22, 2023
11 of 12 checks passed
@osma osma deleted the issue1484-concept-page-templates-cypress branch November 22, 2023 09:37
@osma osma modified the milestones: 3.x, 3.0 Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

3 participants