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

Full width search bar + explicit action buttons for search facets #1434

Merged
merged 17 commits into from
May 21, 2024

Conversation

Ingvord
Copy link
Member

@Ingvord Ingvord commented Mar 25, 2024

Sync up master with release_search_ui

This one will basically introduce full width search bar, instead of having it in the facets, see #1147

@Ingvord Ingvord changed the title Full width search bar (#1426) Full width search bar + explicit action buttons for search facets Apr 22, 2024
@Ingvord
Copy link
Member Author

Ingvord commented May 11, 2024

I believe the remaining failing test is elastic search bound:

index-ec1c3da3.js:133634 CypressError: `cy.request()` failed on:

http://localhost:3000/api/v3/elastic-search/delete-index?index=test

The response we received from your web server was:

  > 400: Bad Request

This was considered a failure because the status code was not `2xx` or `3xx`.

If you do not want status codes to cause failures pass the option: `failOnStatusCode: false`

-----------------------------------------------------------

The request we sent was:

Method: POST
URL: http://localhost:3000/api/v3/elastic-search/delete-index?index=test
Headers: {
  "Connection": "keep-alive",
  "Authorization": "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJfaWQiOiI2NjNmMmRlNTY4NDczOWZmZGYwZWMxMmUiLCJ1c2VybmFtZSI6ImFkbWluIiwiZW1haWwiOiJhZG1pbkB5b3VyLnNpdGUiLCJhdXRoU3RyYXRlZ3kiOiJsb2NhbCIsIl9fdiI6MCwiaWQiOiI2NjNmMmRlNTY4NDczOWZmZGYwZWMxMmUiLCJpYXQiOjE3MTU0MTY2MDAsImV4cCI6MTcxNTQyMDIwMH0.X6V5RR6sSXEdy7QkaLsko4p1q6_Y3ucvz_IiwKi23YM",
  "Content-Type": "application/json",
  "user-agent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/124.0.0.0 Safari/537.36",
  "accept": "application/json",
  "accept-encoding": "gzip, deflate",
  "content-length": 0
}

-----------------------------------------------------------

The response we got was:

Status: 400 - Bad Request
Headers: {
  "x-powered-by": "Express",
  "access-control-allow-origin": "*",
  "content-type": "application/json; charset=utf-8",
  "content-length": "139",
  "etag": "W/\"8b-GT2GRHXQxRi8Bfyq4HNnE6sHijM\"",
  "date": "Sat, 11 May 2024 08:36:40 GMT",
  "connection": "keep-alive",
  "keep-alive": "timeout=5"
}
Body: {
  "statusCode": 400,
  "message": "deleteIndex failed-> ElasticSearchService TypeError: Cannot read properties of undefined (reading 'indices')"
}


Because this error occurred during a `after each` hook we are skipping the remaining tests in the current suite: `Elastic search`
    at <unknown> (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:133028:72)
    at tryCatcher (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1807:23)
    at Promise._settlePromiseFromHandler (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1519:31)
    at Promise._settlePromise (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1576:18)
    at Promise._settlePromise0 (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1621:10)
    at Promise._settlePromises (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1701:18)
    at <unknown> (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:2392:25)
From previous event:
    at Promise.longStackTracesCaptureStackTrace [as _captureStackTrace] (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:3486:19)
    at Promise._then (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1239:17)
    at Promise.then (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1132:17)
    at next (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:144353:64)
    at <unknown> (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:144374:16)
    at tryCatcher (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1807:23)
    at Promise._settlePromiseFromHandler (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1519:31)
    at Promise._settlePromise (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1576:18)
    at Promise._settlePromise0 (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1621:10)
    at Promise._settlePromises (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:1701:18)
    at <unknown> (http://127.0.0.1:4200/__cypress/runner/cypress_runner.js:2392:25)
From Your Spec Code:
    at Context.eval (webpack://scicat-frontend/./cypress/support/commands.js:371:7)

Let's discuss this at the upcoming meeting

@Junjiequan Junjiequan self-requested a review May 14, 2024 07:30
Copy link
Member

@Junjiequan Junjiequan left a comment

Choose a reason for hiding this comment

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

I tested the changes locally and I see that the search query sends only fullquery request, where the fullfacet request is also needed to show the count number correctly.
The e2e fail I believe is due to the UI changes. The fix is to include steps to trigger search before checking the search result.

@Ingvord
Copy link
Member Author

Ingvord commented May 15, 2024

@Junjiequan I have implemented a few fixes including facets count and clicking the search button, however elastic search still fails. Will it be possible to have a zoom chat, so I can learn how to setup and run elastic search tests locally? Thanks

@Junjiequan
Copy link
Member

Junjiequan commented May 16, 2024

@Junjiequan I have implemented a few fixes including facets count and clicking the search button, however elastic search still fails. Will it be possible to have a zoom chat, so I can learn how to setup and run elastic search tests locally? Thanks

Yes I will be available all day tomorrow or next week after Monday if you prefer. [email protected] is my email you can send me an invite.

One of the common elasticsearch test failing issue I face for local setup, is due to the wrong admin & archiveManager credentials in the root cypress.config.ts that does not align with the credentials stored in DB

Copy link
Member

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

@Ingvord after I reviewed this PR with @Junjiequan, I realized that there are some open issues that I did not consider.
Please post on the PR

@nitrosx nitrosx self-requested a review May 16, 2024 13:20
@nitrosx
Copy link
Member

nitrosx commented May 16, 2024

This PR initiated a discussion in our team. @Junjiequan and myself had a brainstorm session while reviewing your design.
The current design and the full width search bar is not using efficiently the space on the page and, as consequence, there is plenty of space not used.

I collected thoughts and suggestions and I took the liberty to come up with a proof of concept for the revised design of the datasets page.

datasets_list_page_design

I will ask feedback from the community.
@Ingvord I was wondering what we can implement as a part of this PR while you address the comments prior being approved and merged in master

@Ingvord
Copy link
Member Author

Ingvord commented May 16, 2024

@nitrosx , @Junjiequan Thanks for your efforts dedicated to this PR! First of all, I have finally managed to deal with the test.

Concerning the design. I do not have strong opinion on that, to my taste though your proposal seems a bit too dense. I mean I sit in front of 4K display and do not feel wasted space. But I agree that the design is not final, and this PR is more about functionality not look-n-feel.

So I agree - let's ask the community. But I would love to merge this one anyway, and then create another PR focused on the design aspect. Otherwise we risk to stuck in an endless limbo of moving inputs and buttons around ;)

Your thoughts?

@Ingvord Ingvord requested a review from Junjiequan May 16, 2024 14:53
@nitrosx
Copy link
Member

nitrosx commented May 16, 2024

@Junjiequan will review the PR tomorrow and if nothing major comes up, we will merge, although we should work quickly to address the address the need to optimize the space usage.

I created a new issue to collect feedback about the image that I posted above: #1479

@Junjiequan
Copy link
Member

I have two comments would love to hear your thoughts before we merge this.

  1. Wouldn't it be better to fully stretch the search bar + button to align with content width below? I agree that we should make major layout improvements in another PR.
  2. Two clear button works differently. one resets dataset table, the other one clear filter texts only. I wonder wouldn't it be better that both buttons work the same.

@Ingvord
Copy link
Member Author

Ingvord commented May 17, 2024

@Junjiequan thanks for your comments!

  1. The truth is that css is still a voodoo magic for me. If you could commit directly into this branch the way you want it, that would be great! You will save tons of my nerve cells, and I will learn something 🤠

  2. As a user I would be confused if innocent clear action on the input will reload the whole table. Maybe we should name the other button "Reset" or something, to avoid mental mapping between these two

@Ingvord
Copy link
Member Author

Ingvord commented May 21, 2024

Thanks @Junjiequan

Copy link
Member

@Junjiequan Junjiequan left a comment

Choose a reason for hiding this comment

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

image

Here is the screenshot of the css changes. And in the commit, I've also done refactoring slightly, so it follows the pattern of the other components.
If @Ingvord & @nitrosx also think it's ready then maybe we can merge it.

Ingvord added 13 commits May 21, 2024 11:18
* First steps

* Beautify

* Use shared search bar component

* Remove search bar from facets

* Some clean ups

* Some clean ups

* Some clean ups

* Code formatting
* Change label

* Add button

* Add button

* Full width search bar (#1426)

* First steps

* Beautify

* Use shared search bar component

* Remove search bar from facets

* Some clean ups

* Some clean ups

* Some clean ups

* Code formatting

* Fix layout

* Add functionality to the button
@Junjiequan Junjiequan force-pushed the release-search-ui branch from b4bf0d6 to 9dcc9af Compare May 21, 2024 09:18
@Junjiequan Junjiequan merged commit f137de8 into master May 21, 2024
6 checks passed
@Junjiequan Junjiequan deleted the release-search-ui branch May 21, 2024 10:43
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.

3 participants