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 mappings: style and visibility fixes #1613

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

osma
Copy link
Member

@osma osma commented Mar 13, 2024

Reasons for creating this PR

The layout and styling of concept mappings was wrong after opening the vocabulary home page and clicking on links that perform a partial page load. Also, when switching between concepts, the mappings didn't update immediately, so the wrong (stale) set of mappings could be shown.

This PR addresses the above problems by adjusting the template structure of the concept-mappings Vue component and making the display of the div element with the actual mappings conditional on the existence of mappings.

Link to relevant issue(s), if any

Description of the changes in this PR

  • make the display of the mappings div conditional on the existence of mappings (using a computed value for clarity)
  • when switching between concepts, clear the old mappings before starting to load new ones so that stale information is not displayed
  • adjust CSS classes of the concept-mappings component

Known problems or uncertainties in this PR

  • This PR is built on top of PR implement fetchWithAbort and use it for partial page load, mappings & alphabetical index #1609 and includes the commits from that PR, so that should be merged first.
  • Should there be Cypress tests specific to these changes or are the existing tests enough?
  • There is now an extra useless div in the DOM structure. I couldn't figure out how to avoid it. I tried changing it to template instead in the Vue template, but it just ended up in the DOM as <template> and broke the functionality.

image

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 Mar 13, 2024
@osma osma self-assigned this Mar 13, 2024
@osma osma requested a review from UnniKohonen March 13, 2024 16:00
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.58%. Comparing base (f22c504) to head (3e66b74).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1613   +/-   ##
=========================================
  Coverage     70.58%   70.58%           
  Complexity     1647     1647           
=========================================
  Files            32       32           
  Lines          4321     4321           
=========================================
  Hits           3050     3050           
  Misses         1271     1271           

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

Copy link
Contributor

@UnniKohonen UnniKohonen left a comment

Choose a reason for hiding this comment

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

Looks good to me! Some kind of indication that the mappings are being loaded could be added but not necessarily in this PR.

There is also a very minor issue with the mappings variable but it doesn't affect functionality.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@osma
Copy link
Member Author

osma commented Mar 14, 2024

Fixed the mappings initialization and added the spinner to the TODO list in the conccept page issue. Merging this now.

@osma osma merged commit 3925c71 into main Mar 14, 2024
12 checks passed
@osma osma deleted the issue1484-concept-page-mappings-style-visibility-fixes branch October 10, 2024 13:23
@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.

2 participants