ProjectMirador / mirador-dl-plugin

a Mirador 3 plugin that adds manifest-provided download links to the window options menu
https://mirador-download-plugin.netlify.com/
Other
6 stars 7 forks source link

Use MUI sizing for dialog, fixes #3 #15

Closed mejackreed closed 5 years ago

mejackreed commented 5 years ago

While this doesn't use the specified 400px it instead uses the build in material-ui responsive sizes. The next one up sm is 600.

@ggeisler is that ok? See maxWidth here: https://v3-9-0.material-ui.com/api/dialog/

ggeisler commented 5 years ago

@mejackreed Yeah, I understand about the built-in breakpoints and in general I agree it makes sense to stick to those whenever possible.

In this case, I think we (I don't remember if this is a decision I made on my own or if @jvine also thought it was a good idea) specified 400px because we were worried that at 600px there is a lot of unnecessary whitespace (except in cases where the canvas title is long) and that it puts the Close button quite a distance to the right from the main dialog content:

Screen Shot 2019-06-07 at 11 02 27 AM

I guess I don't feel super strongly about it and could live with 600px if you think customizing to 400px adds too much complexity or confusion to the code, but just from a UX and visual design point of view, 400px is better and was an intentional design decision.

(Probably not relevant but just in case, when I go to the Netlify preview of this PR the dialog is not 600px, it seems to be 360px? So I'm not sure if I'm completely missing something here or what.)

mejackreed commented 5 years ago

@ggeisler Sorry didn't make it clear in this PR, I'm using the xs breakpoint which is 360.

Is the 360 an ok approximation of 400? I went with this so we could get the baked in responsiveness things

360

Screen Shot 2019-06-07 at 12 11 56 PM

400

Screen Shot 2019-06-07 at 12 13 16 PM

ggeisler commented 5 years ago

Oh okay, I see. Sure, 360px is fine and makes sense in this case to stick with the built-in breakpoints. 👍