Modal updates

[kimispencer]

!!! Breaking changes

• Add an extraExtraLarge size variant
• Update modal title to accept JSX over String
• Remove rounded borders on full width/height mobile modals

[maddie927]

This feels a little odd to me. What do you think of this vs an additional constructor like XXLarge or AbsurdlyLarge?

[maddie927]

I think part of why it feels weird is that Nothing and Just { extraExtraLarge: false } are two different ways to express the same state.

[kimispencer]

Yeah... I originally added an XXLarge constructor, but then facing all the breaking changes (and handling all the cases on where we use size for different components felt tedious, and I was hoping this would be a simpler/more direct solution) which is why I choose this path. The Maybe was also added just because I wanted to use ExtraLarge Nothing; but maybe this wasn't the right approach.

[hdgarrood]

You could do ExtraLarge { extraExtraLarge :: Boolean } and still match on it using ExtraLarge _ though, surely?

Anyway I think I would prefer adding an ExtraExtraLarge constructor here, as although adding an extra constructor may require more work inside lumi-components, I think it will be less work for downstream users, as this way the current ExtraLarge constructor will continue to work without needing modification. If we go the extra constructor route, I think the only place things will break is when people are pattern matching on Size, and I doubt that's happening very much outside of this library. I think prioritising downstream users makes the most sense here; firstly that's kind of the whole point of reusable libraries (to save work downstream) and also, even if we're only considering our own code in our apps, doing the upgrade in our own apps to handle a new constructor is probably less work than adding an argument to the existing ExtraLarge constructor which we are already using in quite a few places.

[kimispencer]

Fair enough, I will update accordingly.

[kimispencer]

Also, with ExtraExtraLarge we have to make some assumptions what to do on that case (which isn't defined in the designs); currently I'm just using the same rules that are applied to ExtraLarge. (For example, with the Button what do we display when it's size is set to ExtraExtraLarge? For now, I am just using the same CSS as ExtraLarge) This is what I was trying to avoid with the other approach, but again see the points against it.

[hdgarrood]

I think that's an issue we run up against either way though, because the previous ExtraLarge (Just { extraExtraLarge: true }) still means something different from ExtraLarge Nothing, right? Ignoring the argument to ExtraLarge with the previous code is just as questionable as treating ExtraExtraLarge the same as ExtraLarge, I think. In my view the real problem here is that we have a single one-size-fits-all Size type which is used across many different components. (Although I am not going to suggest changing that because I guess it would be a lot of effort for maybe not that much benefit.)

[kimispencer]

Yeah, it might be worth reconsidering how we use Size across many different components; but agree that should be dealt with separately, and might not be worth the effort to benefit ratio. Just something to note, ExtraExtraLarge on certain components will equate to ExtraLarge CSS rules.

[hdgarrood]

Just something to note, ExtraExtraLarge on certain components will equate to ExtraLarge CSS rules

Yeah that approach sounds good to me!