-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
There was a problem hiding this 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).
apps/datahub/src/app/common/footer/mel-datahub-footer.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
apps/datahub/src/app/common/footer/mel-datahub-footer.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/common/footer/mel-datahub-footer.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/common/footer/mel-datahub-footer.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/common/footer/mel-datahub-footer.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/common/footer/mel-datahub-footer.component.html
Outdated
Show resolved
Hide resolved
bf03201
to
3e5c451
Compare
There was a problem hiding this 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
apps/datahub/src/app/common/footer/mel-datahub-footer.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
apps/datahub/src/app/common/footer/mel-datahub-footer.component.html
Outdated
Show resolved
Hide resolved
1be23c1
to
f9454fd
Compare
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 :