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

landing page: add blr related works section #674

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

jennur
Copy link
Member

@jennur jennur commented Dec 8, 2023

Closes https://github.com/zenodo/rdm-project/issues/567

Needs inveniosoftware/react-searchkit#260

Edit: After discussing with Lars, it was decided to remove the search bar for now, as there are issues related to having the initial query string (to show only related results for the specific record) + additional query strings added by the user. In order to add the search bar back, these are the known alternatives that will need a solution:

  • if the query string is added to the state (queryString), the string shows up in the input field. The input field should be empty, but the query string needs to remain.
  • if the query string is added directly to the endpoint url parameters, the search does not work, as there will be two q parameters once another search is performed.
  • the query string cannot be set as a filter, as this requires the implementation of custom filters in the backend.

Screenshots

Screenshot 2023-12-13 at 8 23 23 AM Screenshot 2023-12-13 at 8 23 33 AM Screenshot 2023-12-13 at 8 26 09 AM

import { PropTypes } from "prop-types";
import { Input, Icon } from "semantic-ui-react";

export const BlrSearchBar = ({
Copy link
Contributor

@kpsherva kpsherva Dec 12, 2023

Choose a reason for hiding this comment

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

is this component any different that the default search bar? maybe I am missing context but my first thought is this PR recreates the whole search app and duplicates a lot of code. We could possibly use overridable features like in other searches.
I don't see any major layout difference with other searches, but correct me if I am missing something

Copy link
Member Author

@jennur jennur Dec 12, 2023

Choose a reason for hiding this comment

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

You're right, the search bar override is not needed I think. I think I can solve it with the props only. Thanks!

Do you mean the search app from invenio-search-ui? I'm overriding some ReactSearchKit components, but they would have to be overridden in any case, as the layout is not the same. I'm not sure I see the benefit of using invenio-search-ui here?

@jennur jennur force-pushed the blr-related-works branch 3 times, most recently from dc0ea53 to d62d05f Compare December 13, 2023 16:27
@jennur jennur marked this pull request as ready for review December 13, 2023 16:38
@jennur jennur force-pushed the blr-related-works branch 5 times, most recently from 8325992 to 82d70a3 Compare December 13, 2023 17:33
Copy link
Contributor

@jrcastro2 jrcastro2 left a comment

Choose a reason for hiding this comment

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

LGTM!
Just left a minor comment about the filter, the PR also need to be rebased there are conflicts.


export const Filter = ({ valuesCmp }) => {
return (
<Dropdown text="Filter by type" button>
Copy link
Contributor

@jrcastro2 jrcastro2 Dec 18, 2023

Choose a reason for hiding this comment

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

Comment: By using the text prop, the user will not see what type of filter is selected unless (s)he opens the dropodwn. I find it more intuitive if once we have selected the filter it's value is displayed.

Suggested change
<Dropdown text="Filter by type" button>
<Dropdown placeholder="Filter by type" button>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I do agree, although this dropdown is a bit different since it works like the filters on the regular search (with checkboxes), so multiple options can be checked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right! Good point! In that case I would just display some text mentioning how many filters are selected like "X filters selected" so that the user is aware that there are filters selected when the dropdown is closed. However this can be addressed in another issue, if we do so please create the issue!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I will test it!

@jennur jennur force-pushed the blr-related-works branch 2 times, most recently from 61893b9 to 4bb99cd Compare December 21, 2023 15:27
@jennur
Copy link
Member Author

jennur commented Dec 22, 2023

Update

  • Added counter for selected filters
Screenshot 2023-12-21 at 2 58 32 PM Screenshot 2023-12-21 at 2 58 26 PM Screenshot 2023-12-21 at 2 58 20 PM

@anikachurilova anikachurilova force-pushed the blr-related-works branch 6 times, most recently from 3986126 to d4d71c7 Compare January 19, 2024 15:52
Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

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

If meant to be reusable, it should be moved to InvenioRDM (maybe later?) and naming should be changed to be more generic (BLR -> Linked records).

@@ -138,4 +138,9 @@ a.inverted {
.ui.medium.header {
font-size: 1.45rem;
}
}

.image.thumbnail-image {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this works well in mobile version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!
Screenshot 2024-01-22 at 10 48 37


return (
<>
<Header as="h2">Linked records</Header>
Copy link
Contributor

Choose a reason for hiding this comment

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

Will Zenodo be available in multiple languages at some point? We might have to mark all strings as translatable.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I remember, it was a problematic thing to implement... but anyway that would be another task to go around and make all the strings translatable, for now doing that would be useless


return (
<>
<Header as="h2">Linked records</Header>
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be maybe moved to Jinja (if easy) to avoid the page flickering.

@anikachurilova anikachurilova force-pushed the blr-related-works branch 3 times, most recently from b7605d7 to 736cfe4 Compare January 22, 2024 13:17
Copy link
Member

@slint slint left a comment

Choose a reason for hiding this comment

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

Looks good overall, some minor comments and/or things to shelve.

Sign-off: @carlinmack @slint

@anikachurilova anikachurilova force-pushed the blr-related-works branch 3 times, most recently from a1ba60d to 86c460d Compare January 24, 2024 14:01
* search for related records only within BLR community
* adjust empty search results to absence of search bar
* add a new loader placeholder
@slint slint merged commit b5268aa into zenodo:master Jan 25, 2024
3 checks passed
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