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] : Footer #13

Merged
merged 5 commits into from
Feb 16, 2024
Merged

[Datahub] : Footer #13

merged 5 commits into from
Feb 16, 2024

Conversation

cmoinier
Copy link
Contributor

@cmoinier cmoinier commented Feb 9, 2024

This PR adds the footer to the MEL datahub pages.

The footer is very similar to the one at https://www.lillemetropole.fr/ .
The differences are :

  • The 'Contactez-nous' button doesn't link to the MEL page, but emails to a specified hard-coded address.
  • The partners icons are different (asked by client), and the original text and icon are removed (the render is different from the mockups)
  • The cookies tab is gone.

image

@cmoinier cmoinier changed the title [Datahub [Datahub] : Footer Feb 9, 2024
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 for the nice footer @cmoinier !

I'm just noticing two things when testing:

  • The footer appears much higher than on https://www.lillemetropole.fr and even identical logos (like social media) appear bigger. I think decreasing the size of the two centered logos would also help decreasing the height at the same time.
  • When loading the app, the footer appears quickly because the page content has not fully loaded yet. Maybe we could improve this behavior somehow (perhaps with a min-heigth somewhere).

Copy link
Contributor

@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.

Thanks @cmoinier, this looks good already!

I have some comments about the styling but some of them are personal preference.
The main thing I noticed are the effects on the icons as well as the links at the bottom that are missing. Also important are the translations for the text that should be added to the translation files.
Let me know if something is not clear.

@cmoinier cmoinier force-pushed the footer branch 2 times, most recently from bf03201 to 3e5c451 Compare February 13, 2024 08:20
Copy link
Contributor

@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.

Thanks @cmoinier for addressing all the comments. For me this looks good to be merged :)

Two last things I would implement:

  • underline icons on hover
  • height of footer in responsive view

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, @cmoinier, looks really nice now! Just left a last suggestion for the padding.

@cmoinier cmoinier force-pushed the footer branch 3 times, most recently from 1be23c1 to f9454fd Compare February 16, 2024 09:00
@cmoinier cmoinier merged commit 2cb38f7 into main Feb 16, 2024
7 checks passed
@cmoinier cmoinier deleted the footer branch February 16, 2024 17:33
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