feat: ✨ Create BackToTop component - #139
Conversation
garronej
left a comment
garronej
left a comment
There was a problem hiding this comment.
Thanks a lot for your work, here is my review.
| const { t } = useTranslation(); | ||
| const { anchor = "#top", right = false } = props; | ||
| return ( | ||
| <div className={right ? cx(fr.cx("fr-grid-row", "fr-grid-row--right")) : ""}> |
There was a problem hiding this comment.
| <div className={right ? cx(fr.cx("fr-grid-row", "fr-grid-row--right")) : ""}> | |
| <div className={!right ? undefined : fr.cx("fr-grid-row", "fr-grid-row--right")}> |
| const { useTranslation, addBackToTopTranslations } = createComponentI18nApi({ | ||
| "componentName": symToStr({ BackToTop }), | ||
| "frMessages": { | ||
| "page_top": "Haut de page" |
There was a problem hiding this comment.
| "page_top": "Haut de page" | |
| "page top": "Haut de page" |
There was a problem hiding this comment.
We do not need the keys to be valid variable names. They arent for other components.
| ); | ||
| }) | ||
| ); | ||
|
|
There was a problem hiding this comment.
We also want export default
| export default BackToTop; |
9f5b04b to
968379d
Compare
garronej
left a comment
garronej
left a comment
There was a problem hiding this comment.
Thank you for your work, here is my review
| import { createComponentI18nApi } from "./i18n"; | ||
|
|
||
| export type BackToTopProps = { | ||
| anchor?: string; |
There was a problem hiding this comment.
If no anchor is explicitely provided the default behaviour shoud be to scroll sroll smoothly to the top of the page.
In this case you'll have to use a <button /> component since it's not accible to use a <a /> without a valid href. We need to apply the fr-link class to the button to make it look a <a />.
Beside it's ok accept a css selector but the recomended approach should be to provide ref. Dsfr components do not allow to provide html id but all enable to provide ref.
|
|
||
| export type BackToTopProps = { | ||
| anchor?: string; | ||
| right?: boolean; |
There was a problem hiding this comment.
| right?: boolean; | |
| /** Default: false (the back to top button is centered) */ | |
| right?: boolean; |
|
|
||
| export default meta; | ||
|
|
||
| export const Default = getStory({}); |
There was a problem hiding this comment.
It would be best to have a story for when the back to top is to the right.
garronej
left a comment
garronej
left a comment
There was a problem hiding this comment.
Hello @sylvainlg,
This has to be adressed: #139 (comment)
|
@sylvainlg Cette PR est ouverte depuis juin 2023 avec trois revues « changes requested » de @garronej, dont la dernière du 08/07/2023 pointait un seul point restant : #139 (comment) Ce point n'a jamais été traité. Pour mémoire, il demandait que sans ancre explicite, le comportement par défaut soit un scroll fluide vers le haut de page — ce qui impose un @sylvainlg comptez-vous reprendre cette PR ? Si oui, elle a besoin d'un rebase sur Si vous n'avez plus la disponibilité, dites-le simplement — le composant reste utile et quelqu'un peut repartir de votre travail. @garronej dans ce cas, faut-il fermer cette PR et ouvrir une issue « Create BackToTop component » qui reprenne vos retours de 2023 ? |
|
Bonjour Oui désolé, j'ai changé de projet depuis, vous pouvez bien entendu reprendre ce qui est exploitable pour rendre disponible le composant avec les dernières évolutions de la lib. |
|
Merci @sylvainlg, et merci d'avoir pris le temps de répondre après tout ce temps. J'ai repris votre travail dans #516 : la structure, le nommage des props et vos deux stories y sont conservés. La seule remarque restée ouverte ici, #139 (comment), est traitée par une prop Le défaut reste l'ancre Je ferme cette PR au profit de #516. N'hésitez pas à y jeter un œil si le sujet vous intéresse encore. |
3 participants
No description provided.