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

First implementation of vocabulary search bar Skosmos 3 #1551

Merged
merged 14 commits into from
Nov 23, 2023

Conversation

joelit
Copy link
Contributor

@joelit joelit commented Nov 16, 2023

Reasons for creating this PR

Draft PR for search field

Link to relevant issue(s), if any

Description of the changes in this PR

This PR implements the following from the linked issue:

  1. General functionality
    1.1 Twig-templates, Bootstrap-layout and CSS
    1.2 Search field can be emptied by clicking X
    1.3 Search can be executed with a chosen content language, or with all languages
    1.3.1 Cypress tests for this
  2. Language options
    2.2 Contents for the language menu are defined in the SKOSMOS JS-object
    2.2.1 Cypress tests for this
    2.3 All UI text is in English
  3. Accessibility
    4.1 Search field can be operated with keyboard
    4.3 Language menu, search field, autocomplete list and search button are structured together

Known problems or uncertainties in this PR

This PR does not implement:

  • Handling of the cookies
  • Triggering partial page loads for changing the content language from the search menu
  • Autocomplete dropdown menu

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)

@joelit joelit added this to the 3.0 milestone Nov 16, 2023
@joelit joelit self-assigned this Nov 16, 2023
@joelit joelit marked this pull request as ready for review November 22, 2023 11:34
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e5db224) 70.53% compared to head (eded339) 70.53%.

Additional details and impacted files
@@             Coverage Diff              @@
##             skosmos-3    #1551   +/-   ##
============================================
  Coverage        70.53%   70.53%           
  Complexity        1643     1643           
============================================
  Files               32       32           
  Lines             4313     4313           
============================================
  Hits              3042     3042           
  Misses            1271     1271           

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

return new bootstrap.Popover(popoverTriggerEl)
})
</script>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the search tips to work as a clickable popover element in Bootstrap. I did not include the search tips box in this PR yet.

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it from this PR as it's not yet needed

cy.wrap(optionValue).should('eq', expectedLanguage);
});
})
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, this kind of commented test should not be in the PR. In this case, I left the WIP in here in case I cannot attend during the rest of the sprint, and somebody wants to have a go with writin the test.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

Looks like a good start.

Regarding the layout/style, this doesn't follow the design exactly. The border is too dark, and I don't think we want rounded corners in the dropdown menu:

image

You mentioned that "Search field can be emptied by clicking X" is implemented, but I didn't see any such feature.

Here are problems that should either be fixed in this PR or added to the list of requirements in the issue #1514 so that they can be addressed later:

  • border color should follow design spec
  • dropdown style should match design expectations (no rounded borders)
  • language names should be shown, not language codes
  • in general, UI texts should be translated (currently a problem for all Vue components)

@joelit
Copy link
Contributor Author

joelit commented Nov 22, 2023

It seems I need to adjust the behaviour of the search form. Chrome looks like this:

Screenshot from 2023-11-22 14-13-25
Screenshot from 2023-11-22 14-13-35

Apparently the rendering of the options-menu of the form is up to the browser ( -_- ), and different browsers yield different results. It could be that the menu should intead be implemented in a more ad-hoc manner to get more control over the visuals.

Same is probably true for the reset text 'x' icon. I'll investigate.
I added the "should follow visual spec" to the list of requirements in the issue and added the language codes to the strings that need to be translated.

@osma osma force-pushed the issue1514-search-bar branch from 0fc277c to 8691d10 Compare November 23, 2023 08:36
@osma osma changed the title Issue1514 search bar First implementation of vocabulary search bar Skosmos 3 Nov 23, 2023
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

LGTM! Can be merged after CI tests have passed

@osma osma merged commit 05968aa into skosmos-3 Nov 23, 2023
12 checks passed
@osma osma deleted the issue1514-search-bar branch November 23, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
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