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

Add ElasticUI framework for client #16

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Add ElasticUI framework for client #16

merged 6 commits into from
Sep 17, 2024

Conversation

bryophyta
Copy link
Contributor

@bryophyta bryophyta commented Sep 12, 2024

What does this change?

  • Install Elastic UI (EUI) and dependencies
  • Replace existing client functionality with EUI components.

How to test

How can we measure success?

Have we considered potential risks?

Images

Before After
image image

Accessibility

@bryophyta bryophyta changed the title Pf/add eui Add ElasticUI framework for client Sep 16, 2024
@bryophyta bryophyta marked this pull request as ready for review September 16, 2024 11:49
@bryophyta bryophyta requested a review from a team as a code owner September 16, 2024 11:49
@bryophyta bryophyta force-pushed the pf/add-eui branch 2 times, most recently from 7d28d31 to ad04353 Compare September 16, 2024 11:51
Copy link
Contributor

@twrichards twrichards left a comment

Choose a reason for hiding this comment

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

brilliant 🙌

newswires/app/db/FingerpostWireEntry.scala Outdated Show resolved Hide resolved
newswires/client/src/NavigationList.tsx Outdated Show resolved Hide resolved
@twrichards
Copy link
Contributor

think search might be a bit broken
image

@bryophyta
Copy link
Contributor Author

bryophyta commented Sep 16, 2024

think search might be a bit broken image

Thanks for this! I haven't been able to reproduce yet though..

When you were running locally, has the pg_trgm extension been enabled in the local db? That's a possible source of it breaking like this. If that's not it, are you able to take a look at the error that's being sent back from the server? (The JSON parsing issue will probably because Play's sending back HTML for an error page.)

@bryophyta
Copy link
Contributor Author

@twrichards I've removed the comments & code that weren't meant to be committed -- thanks for catching those!

It all seems to be working okay for me locally and I haven't been able to reproduce the issue you were having with search. Perhaps we can pair tomorrow on trying to reproduce?

@twrichards
Copy link
Contributor

think search might be a bit broken image

Thanks for this! I haven't been able to reproduce yet though..

When you were running locally, has the pg_trgm extension been enabled in the local db? That's a possible source of it breaking like this. If that's not it, are you able to take a look at the error that's being sent back from the server? (The JSON parsing issue will probably because Play's sending back HTML for an error page.)

Yeah it's the pg_trgm thing, though I had created the DB fresh, so I assumed the command added to docker-compose.yml in #15 would do it. Interestingly when I open the postgres terminal via docker desktop to do it manually after the fact, I get the following when I try to run the command as is, I wonder if the same thing is happening when DB is run fresh?
image

@bryophyta
Copy link
Contributor Author

We're confident that the issue with search locally isn't a regression here. We're going to add a fix here, but in the short term if you run this in the newswires db container it should unblock locally: psql -U postgres -d newswires -c "create extension if not exists pg_trgm;"

@bryophyta bryophyta merged commit c2a1268 into main Sep 17, 2024
3 checks passed
@bryophyta bryophyta deleted the pf/add-eui branch September 17, 2024 10:53
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.

2 participants