New TPR service list component with search #494
Conversation
…al-hdbt into service_list_search
Added twigs for list search and included new teaser view mode
* updated teaser search result design * added margin-top for tags * adjustments for small screens * fixed design * removed commented code * Several fixes to tpr search result new feature Co-authored-by: Dragos Dumitrescu <[email protected]>
There was a problem hiding this comment.
Good job, I see that you have taken a look at how we work with this theme. I was surprised how many of our conventions you were able to use without us going through them specifically!
There are still some changes that need to be done. It seems that you received the wrong designs, as we have recently unified the card design in many areas. The new unified card designs can be seen here: https://app.abstract.com/projects/6a2238d2-fcb7-40e4-b2aa-a4487941ad8a/branches/main/collections/b41c3fd8-6964-4079-b38f-f99ed1df568c
The new card styles your code is supposed to be using can be seen here: https://share.goabstract.com/b9df65a4-f83d-4299-b9bf-b9f3a35c0662?collectionLayerId=88bf4c41-c59b-4007-9859-4487054bacfe&mode=design
We have written a new unified card template to card.twig and an example of its usage can be seen at tpr-unit--minimal.html.twig and tpr-unit--teaser-with-image.html.twig. The unified theme should cater to all your card needs in this case and if changes are needed to the unified styles, please contact us so that we can make the changes and consider all twig and react template affects.
I have now commented on issues regarding the SCSS problems here, so that you better know in future how we have agreed to work with styles and themes on this project. These comments are mostly so that you understand our workflow in future. Please contact us if you have any questions. One thing that we strongly recommend is getting the stylelint linter working in your IDE, as it seems there are plenty of linting issues. For example, here's one file linter results:
You probably should first switch to use the new card styles before fixing these problems as the unified card styles will make most of the css code you wrote obsolete and as such should be deleted.
Also, another thing, I spotted an block-1 reference, please use descriptive names for blocks as it reduces possible errors and miscommunication.
Anyways, I'm sorry that our codebase is not as clean and contains problems we're trying to refactor away. That's why some of the conventions I see here come from our old code that has not yet been cleaned. Also it's a pity that you received the old card styles and did not get information that we were working on unfied styles. These were totally not on you but on us and the City lack of communication. I hope we can improve the communication in the future and work better together. ❤️
I can be found from the city slack and elsewhere too, please contact me for further questions and collaboration.
- Mikko Tapionlinna
| 'views', | ||
| 'views--' ~ id|clean_class, | ||
| 'views--' ~ display_id|clean_class, | ||
| dom_id ? 'js-view-dom-id-' ~ dom_id, |
There was a problem hiding this comment.
this js-view-dom-id- is required for ajax pagination / filtering, beign used by Drupal Views Ajax library
templates/views/views-view-unformatted--service-list--block-1.html.twig
Outdated
Show resolved
Hide resolved
|
@Arkkimaagi thank you for your in depth review and pointing me to the card.twig - with that, I removed lots of scss and markup. Now it's a smaller PR to review. We have taken a look on https://www.hel.fi/en/health-and-social-services/health-care/health-stations when started this card, but at that time it wasn't refactored with the new card. Regarding style lint and class related comments, I have also corrected the existing twig and scss for this existing component, to pass the stylelint - https://www.hel.fi/en/decision-making/contact-the-city-of-helsinki/advisory-services |
There was a problem hiding this comment.
Good job, much less code than before. There were a few things still to check. I had some trouble getting this to run, but Juho will write about that in a bit. When we got the code running, the editor view was somewhat confusing. We mentioned this to design team and we recommend that you work together with them to improve the editor UX so that people understand how to use this even after the current editors have moved on etc.
So, please take a look at the comments below and check with the designers about the editor UX.
| .service__title { | ||
| @include font('h4'); | ||
| margin: 0; | ||
| } | ||
|
|
||
| .hel-icon { | ||
| --icon-size: #{$teaser-icon-size-mobile}; |
There was a problem hiding this comment.
Why was the icon size for mobile removed here? We should not break existing card layouts.
There was a problem hiding this comment.
Have you checked that you did not break mobile layouts for these? To me it seems that you broke the arrows from these cards. See the prod and your branch difference.
| text-decoration: underline; | ||
|
|
||
| .hel-icon { | ||
| transform: translateX($spacing-quarter); |
There was a problem hiding this comment.
Why was this animation size adjusted? Now it seems too small compared to the icon size.
Please do not move code and change code in same commit. Also, please make smaller commits that only change one thing and have a properly formatted message so that we can follow your logic.
By properly formatted message I mean that each commit message should read as if it had this string in front of it:
"If accepted, this commit will: "
Like this:
"Reduce service-link icon animation amount."
There was a problem hiding this comment.
Hello. Will check again the comments and the mobile version.
Mainly the changes you've mentioned in this review of why were done is what I've mentioned in the comment above
Regarding style lint and class related comments, I have also corrected the existing twig and scss for this existing component, to pass the stylelint - https://www.hel.fi/en/decision-making/contact-the-city-of-helsinki/advisory-services One comment here: the animation in the new card.twig is different than current one at https://www.hel.fi/en/decision-making/contact-the-city-of-helsinki/advisory-services I assume is ok to refactor it to use the same new animation
There was a problem hiding this comment.
we've updated the icon for existing card. the transition all was an issue for style lint as well as other styles around that arrow so that's why we removed some styles initially. Now we've refactored the code around that arrow to keep same animation but to pass stylelint checks
There was a problem hiding this comment.
The backend code looks good! However, it would be good to add more detailed steps to the PR review steps. Probably not that relevant for this one anymore, but would be helpful for PRs in the future and should speed up the review process.
For example:
- The platform config PR (New TPR service list component with search drupal-helfi-platform-config#407) should be linked in the "How to install" section, currently it's only listed in the "Other PRs" part which is easy to miss. The actual component is installed through this module, so you can't review this locally if you miss this part
- Instructions should be added on how to install and set up the remote repository modules and themes. This is how we did it, but there might be an easier workflow too:
composer reinstall drupal/hdbt --prefer-source
cd public/themes/contrib/hdbt
git remote add plaenh-185 [email protected]:dragos-dumi-ibm/drupal-hdbt.git
git fetch plaenh-185
git checkout plaenh-185/main
- Additional steps should be added to the "How to test" section:
- Going to the TPR integration list for services (/admin/content/integrations/tpr-service) and publishing some items to test with
- The "Service IDs" filter uses the "Service Grouping IDs" field, which is kind of hard to find. It would be helpful to list some services to publish and then list the related grouping ID.
dragos-dumi-ibm
requested review from
juho-lehmonen and
Arkkimaagi
and removed request for
Arkkimaagi and
juho-lehmonen
There was a problem hiding this comment.
Otherwise code looks good, but the visual style does not match design on certain aspects.
- Background for the search area is not grey
- There's a missing
<hr>between search and search result count.
This is how it looks now:
This is more how it should look like:
I think this can be fixed at least partly by rebasing our hdbt main-branch here and rebuilding css files.
|
Looks good now. Only two steps left before merging, but we will handle those two steps on our end.
|
|
I switched the base to PLAENH-185 branch, so that I can compile the dist-folder before merging on main branch. |
PLAENH-185
New TPR service list component with search
What was done
More details at
https://helsinkisolutionoffice.atlassian.net/browse/PLAENH-185
How to install
make drush-updb drush-crPARTIAL_MIGRATE=1 drush migrate:import tpr_service --no-progressHow to test
Add to a page that has paragraphs the new component "Service List Search"
Configure the component with using service_id and/or specific services
The page used in installation should have a listing that looks like this https://app.abstract.com/share/505f9e4a-9291-48ca-9bcc-4f38a7bc3e36?collectionId=4d66b7af-5224-4b71-8d19-a4df8af377c0&collectionLayerId=0d3595f3-dc9c-4c8e-8dc3-e7c5c09ac932&present=true&preview=false&sha=bf449ac81535612facaff26e7355b3b354509124
Search should work against title, short desc and name synonyms
Services teasers should use the imported translations
Designers review
Other PRs