betagouv / dsfr-view-components

Composants Rails pour le DSFR
https://betagouv.github.io/dsfr-view-components
MIT License
32 stars 4 forks source link

component: modal #149

Closed adipasquale closed 1 year ago

adipasquale commented 1 year ago

cf #85

Note: this is based on #150 as the ModalComponent depends on the ButtonComponent one

image

Screenshot 2023-02-22 at 18 43 46
freesteph commented 1 year ago

cf #85

Note: this is based on #150 as the ModalComponent depends on the ButtonComponent one

* I have not handled the case where you want to use an icon in the title, we can do it later I think

* a modal without an explicit html `id` is pretty much useless since you cannot refer to it in any button to open it. In this PR I’ve left it to the user to pass it with the regular `html_attributes: { id: "modal-1" }` kwarg. I don’t require the presence of this attribute, the component won’t fail if you don’t pass it. It will just be harder to use it (you’d need custom JS). We could instead have an explicitly required `id` param in the component, but it’d be redundant with the `html_attributes` one. I don’t have a strong opinion on this.

I think that's fair. I'm all for helpful interfaces but this is the responsibility of the user, and as you said it's redundant with the html_attributes so I think this is the right way.

* for the buttons slots I’ve made it rather simple : it’s always a buttons group even when there is only a single button. It seems to look identical so it’s easier to code this way 🤷 I was a bit lazy on that one.

Sounds good to me.

* We could in the future have a `cancel_button` slot shortcut that would pre-fill the `data-fr-opened` and `aria-controls` attributes.

* I don’t really know what kind of API we could think of for the external opening button. maybe an option to the `ButtonComponent` or a variant or a subcomponent that takes a modal component as an argument and auto fills the `data-fr-opened` and `aria-controls` attributes ? That’s a thought for later though.

* in the guide previews I had to hack the panel contents to be higher because the modal pops in INSIDE these panel contents for some reason
adipasquale commented 1 year ago

thanks for the review @freesteph ! However I made some breaking changes in the last commit on which your sharp eye would help

The whole point is to make the modal work with this kind of Turbo Drive flow :

This flow thus works with JS (looks very much like a normal modal behaviour) and without JS with regular page navigations.

this is not so easy as I don’t think that DSFR modals were thought with this kind of html over the wire flows. namely there are no other documented ways to open a modal than to click on a button that has the right html attributes aria-controls and so on

here is what I did to make it work:

  1. I added a opened: true/false kwarg to the modal that will toggle the fr-modal--opened class, which is enough to make it appear on page load
  2. I changed the signature of the button slots : they are not anymore tied to the ButtonComponent. This means that you can call the c.with_button with any arbitrary html content. This is required in order to allow the caller to actually pass a link (a tag) instead of a button so that the cancel button will navigate away from the page.
  3. Added a header slot that takes random html. It can be used to completely override the default close button, again in order to pass a a link that will navigate away to the previous page. I could have used a more specific close_button slot though.

among these 3 changes, number 2 is the most disappointing, it degrades the DX for most cases in order to make it work for this edge-case :/ If you have a better suggestion please let me know.

Screenshot 2023-03-09 at 13 42 11

... and I kept the best for the end and you are going to LOVE this puzzle :

this last commit has broken the guide’s display of modal with links altogether for an unfathomable reason

Screenshot 2023-03-09 at 13 41 57

I am very much sure that this behaviour is limited to the guide. When the dsfr-view-components library is actually required and used in a Rails project, it works - I am currently using this branch in production for Collectif Objets succesfully.

I’ve investigated a bit and I think it comes from the format_haml method that is responsible for rendering the examples in the doc

adipasquale commented 1 year ago

@freesteph I’ll merge this one but don’t hesitate if you have afterthoughts - on the component API and code, not the guide!