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

UHF-7791: Add react search and typescript support #508

Merged
merged 64 commits into from
Feb 2, 2023
Merged

Conversation

Jussiles
Copy link
Contributor

@Jussiles Jussiles commented Jan 2, 2023

UHF-7791

  • Idea is to have all React searches in HDBT because those are frontend and are using styles from HDBT. By having all React apps in one place it is possible to share common code between apps and use same Eslint and Typescript rules.

What was done

  • Add React search support. First search added is Event search.
  • Add common folder for React with unified card components.
  • Add refine search button at the bottom of the listing
  • Add Typescript support. Only used with react at the moment.
  • Eslint extend airbnb instead airbnb-base to enable React and typescript linting. Override linting rules for Typescript.
  • Stop using Create react app (CRA). Instead use HDBT webpack config.
  • Update readme.

How to install

  • Make sure your instance is up and running on latest dev branch.
    • git pull origin dev
    • make fresh
  • Update the HDBT theme
    • composer require drupal/hdbt:dev-UHF-7791-tsconfig
  • Update the Helfi Platform config
    • composer require drupal/helfi_platform_config:dev-UHF-7791-drupal-react-search-module
  • Run make shell
  • Run drush updb && drush cr && drush locale-check && drush locale-update && drush cr

How to test

Test paragraph functionality:

  • Location filter works
  • Daterange filter works
  • Checkbox filters work
  • Pagination works

Designers review

  • This PR does not need designers review
  • This PR has been visually reviewed by a designer (Name of the designer)

Other PRs

@Jussiles Jussiles changed the title UHF-7791: tsconfig WIP: UHF-7791: tsconfig Jan 2, 2023
@Jussiles Jussiles changed the title WIP: UHF-7791: tsconfig UHF-7791: Add react search and typescript support Jan 5, 2023
Copy link
Contributor

@Arkkimaagi Arkkimaagi left a comment

Choose a reason for hiding this comment

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

Great job, and stuff seems to be working nicely. I did spot some nitpick stuff, but those are minor things..

Problem with the react dropdown icon not having an empty alt-attribute
image

Problem also with paging
image

image

src/js/react/apps/linkedevents/components/Pagination.tsx Outdated Show resolved Hide resolved
src/js/react/apps/linkedevents/components/Pagination.tsx Outdated Show resolved Hide resolved
src/js/react/apps/linkedevents/components/Pagination.tsx Outdated Show resolved Hide resolved
src/js/react/apps/linkedevents/components/Pagination.tsx Outdated Show resolved Hide resolved

.hdbt-search__date-input table tbody tr td,
.hdbt-search__date-input table thead th {
min-width: unset;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should get rid of the min-width from table cells alltogether, I do not quite understand why we have it at all. Maybe add an comment that points to UHF-3044 and reference there to remember to remove this line when it's done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, why is this set twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems it's not needed so I'll remove it. Maybe HDS has made some updates since this was added. It's not my code so I don't know more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please link the ticket here both in ticket and in code so that we remember to fix it?

Copy link
Contributor Author

@Jussiles Jussiles Feb 1, 2023

Choose a reason for hiding this comment

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

I removed the min-width from here because seems it's not needed but I can add comment about the ticket if you want to check this still when working with the UHF-3044 ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems I forgot to push code yesterday evening but as said min-width is now removed but added also comment about checking it with 3044.

src/scss/06_components/paragraphs/_event-list.scss Outdated Show resolved Hide resolved
src/scss/06_components/paragraphs/_event-list.scss Outdated Show resolved Hide resolved
src/scss/06_components/paragraphs/_event-list.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@Arkkimaagi Arkkimaagi left a comment

Choose a reason for hiding this comment

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

One last thing... 😢

src/js/react/common/Icon.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@Arkkimaagi Arkkimaagi left a comment

Choose a reason for hiding this comment

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

Good job, huge set of code you've managed to get working! 🚀 Approved!

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.

6 participants