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 search card #10

Merged
merged 12 commits into from
Feb 19, 2024
Merged

Add search card #10

merged 12 commits into from
Feb 19, 2024

Conversation

Angi-Kinas
Copy link
Contributor

@Angi-Kinas Angi-Kinas commented Feb 1, 2024

This PR implements the search card on the search page.
It uses the previously created favorite logic to add/ remove datasets to favorites if logged in.

image

image

@Angi-Kinas Angi-Kinas force-pushed the search-card branch 2 times, most recently from e77b9d9 to e9c3434 Compare February 8, 2024 14:20
add metadata quality indicator
fix rebase issues,
add transaltions.
button styles, make button and image style
more flexible with classes,
use favorite-heart component in search card,
use primary button in dataset page
login and add favorite, show favorite card and
search card
@Angi-Kinas Angi-Kinas marked this pull request as ready for review February 12, 2024 12:47
check css background color instead of class name
Copy link
Member

@tkohr tkohr left a comment

Choose a reason for hiding this comment

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

Thanks @Angi-Kinas. The search page is starting to look nice:-)

Some small things I noticed testing:

  • I wasn't able to select the card with the keyboard
  • The cursor pointer for the card is missing
  • A click on a keyword does not execute a search on the term

<p class="leading-relaxed text-sm text-gray-700 clamp-3">
{{ record.abstract }}
</p>
<div class="flex flex-col h-[72px] justify-between shrink-0">
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the flex-col in the <a> is sufficient as the vertical gap between all the elements of the card seems to be the same (12px btw).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the height? Because if I don't set it, the cards will have different heights like so:

image

It's because some "Producteur" names are longer than others...

Copy link
Member

Choose a reason for hiding this comment

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

I guess you are right, that you need flex-col on different levels. I think we'll need to limit the size of Localisation and Producteur anyhow though, as very long names can still break the layout. What do you think of truncateing them to one line and displaying the tooltip for each entry instead of displaying the abstract tooltip for the entire card?
Limiting all entries of the card should also enable us to make the vertical margins/gaps between them as consistent as possible.

- change name of ButtonPrimary
- change classname
- change some styles in search-card
- make keywords click work
- make key tab work on cards
@@ -1,10 +1,11 @@
<div class="relative">
<div class="grid md:grid-cols-2 lg:grid-cols-3 gap-[30px]">
<div class="flex justify-center flex-wrap gap-6">
Copy link
Member

Choose a reason for hiding this comment

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

Watch out, this will also display 1 or 2 results centered, while I think we'd expect them to show up bound to the left. Maybe use justify-start? What's the reason to change to flex here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right, I saw that the last result was centered alone. I could set a width on the parent component and center this parent component and then use justify-start for the cards, as you said. But not sure if this will look nice on all screen sizes.
The reason for changing this was the responsive behaviour. Somehow the cards got stacked on top of each other before wrapping to the next line. I can change it back though. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, I guess flex is indeed the better way to handle responsiveness here since/if we don't want to reduce the card width. I think we'll also need to use a mel-container-lg lg:mx-auto in the parent component (search-results.component) to center the content and potentially adapt the mel-container-lg width to the mockups to fit three cards.

Copy link
Member

Choose a reason for hiding this comment

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

After thinking about it again, I think it's better user experience to use the grid layout and make the cards themselves responsive, instead of giving them a fixed width. This allows to use the available space no matter what size the screen has (except for very wide ones maybe). I've reworked this in 2622de3.

@tkohr
Copy link
Member

tkohr commented Feb 15, 2024

I've added a couple of commits, mainly reworking the card to be responsive, allowing to display them in grid using a container width that matches MEL's design system.

Also fixed the mel button, when not having a buttonClass value, setting its default to primary style and finished its renaming.

Everything looks good to me now. Thanks again for the work @Angi-Kinas ! I'll let you have a last look at my changes and merge if you are fine with them.

@Angi-Kinas Angi-Kinas merged commit 7f1c104 into main Feb 19, 2024
7 checks passed
@Angi-Kinas Angi-Kinas deleted the search-card branch February 19, 2024 08:50
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