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

[Datahub] [Resdesign] Header section #765

Merged
merged 18 commits into from
Jan 22, 2024
Merged

[Datahub] [Resdesign] Header section #765

merged 18 commits into from
Jan 22, 2024

Conversation

cmoinier
Copy link
Collaborator

@cmoinier cmoinier commented Jan 12, 2024

This PR applies the redesign of the header section, namely :

  • add text to the star toggle
  • redesign and position the language switcher on the top right
  • redesign the back button
  • remove sticky navigation bar
  • redesign the title layout
  • show the dataset type below the title on the left
  • show update date below the title
  • show status below the title

To do :

  • Address review
  • Add condition to language switcher (if in conf)

This is NOT COVERED (will be in another PR):

  • thumbnail
  • "share" link

To test

Dataset /dataset/01491630-78ce-49f3-b479-4b30dabc4c69 has all of the needed parameters of the design.

Screenshot

image

Copy link
Contributor

github-actions bot commented Jan 12, 2024

Affected libs: feature-search, feature-router, feature-map, feature-dataviz, feature-record, ui-catalog, feature-catalog, ui-inputs, feature-editor, ui-elements, ui-search,
Affected apps: datahub, metadata-editor, demo, webcomponents, search, map-viewer, metadata-converter, datafeeder,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@cmoinier cmoinier marked this pull request as draft January 15, 2024 08:01
@cmoinier cmoinier marked this pull request as ready for review January 15, 2024 09:02
@coveralls
Copy link

coveralls commented Jan 15, 2024

Coverage Status

coverage: 82.169% (+0.04%) from 82.13%
when pulling b1d76bb on DH/redesign-header
into 3678ce5 on main.

Copy link
Collaborator

@Angi-Kinas Angi-Kinas left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good already! :-)
Only some minor comments.

translations/en.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@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 for the clean adaptions @cmoinier! There are still some details that don't exactly match the mockups. Could you have a second check on that? Some I see at first glance are:

  • the margin/padding below the "Geographical dataset" etc. line should be slightly larger
  • background color of language switcher should be primary
  • letter spacing of "BACK" should be wider
  • should the time of last update be displayed? (It is not included in mockups)
  • the date is not formatted as in mockups (which is not french btw) and not localized. We should eventually pass the current locale to toLocaleString().

I'm also wondering if there is a specific reason why we need to reload the page when switching the language (cc @fgravin)? The line does not come from this PR, but its effect is visible here.

@cmoinier
Copy link
Collaborator Author

cmoinier commented Jan 15, 2024

Thanks @Angi-Kinas & @tkohr for your helpful reviews 🙂

I made the following changes :

  • Changed displayMap$ name to isGeodata$

  • Changed displayCount type to Boolean and made it optionnal

  • Changed the margin below the "Geographical dataset" etc. line

  • Added the primary color to the language switcher

  • Spaced out the "BACK" letters

  • Formatted the date as in the mockups, but yes, I agree that the date should be in current locale.

  • I didn't add the time of last update, since it's not in the mockups. wdyt @fgravin @jahow ?

@fgravin
Copy link
Member

fgravin commented Jan 15, 2024

I'm also wondering if there is a specific reason why we need to reload the page when switching the language (cc @fgravin)? The line does not come from this PR, but its effect is visible here.

It is because changing the language has many consequences <hich are not handled internally:

  • reload the search as the search is driven by the language (the index language we apply the search on)
  • reload advanced filter values etc...

@fgravin
Copy link
Member

fgravin commented Jan 15, 2024

  • Changed displayMap$ name to isGeodata$

to me this is 2 different things, cf my comments in Slack

@tkohr
Copy link
Collaborator

tkohr commented Jan 17, 2024

Thanks for all the adaptions @cmoinier! The date formats adapts well when switching the language on my side 👍

When using the geo2france config (header_background = "#1f4490" from mockups or the similar value from primary color), I still encounter a couple of problems:

dh_header

  • The favorite star disappears when favoring a record due to used colors
  • "Donnée géographique" is not distinct anymore => should perhaps be bg-primary-darker if this is ok for generic design
  • I'd guess the [] around the status were just to represent a placeholder and can actually be removed

Maybe it'd be nice to add some e2e tests as well.

Copy link
Collaborator

@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 for the continued effort on this @cmoinier ! I let you apply the last suggested change and then it LGTM.

@cmoinier cmoinier merged commit 9ae0a68 into main Jan 22, 2024
8 checks passed
@cmoinier cmoinier deleted the DH/redesign-header branch January 22, 2024 09:05
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.

5 participants