Skip to content
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

Add a BoxShadow component. #205

Merged
merged 3 commits into from
Oct 2, 2022
Merged

Conversation

Dretch
Copy link
Contributor

@Dretch Dretch commented Sep 17, 2022

As proposed in issue #203.

Options for light location (i.e. shadow position)

I don't really have an opinion on this, but here are some options:

Light in the center (original proposal)

light-at-center

Light in the top center (MacOS-ish)

light-at-top-center-mac-os-style

Light in the top-left (Android Material design ish)

light-at-top-left

@@ -142,6 +144,12 @@ buildUI wenv model = widgetTree where
filler
] `styleBasic` [bgColor (grayDark & L.a .~ 0.5)]

confirmDeleteLayer = case model ^.action of
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The todo example now includes a confirm popup when deleting a todo, because the confirm alert was not previously in the examples anywhere (and I wanted to test because I added the shadow to it).

@@ -52,6 +53,7 @@ instance Default Theme where
-- | Default theme settings for each widget.
data ThemeState = ThemeState {
_thsEmptyOverlayStyle :: StyleState,
_thsShadowColor :: Color,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To allow changing the shadow color via the theme.

@@ -237,6 +237,10 @@ newRenderer c rdpr envRef = Renderer {..} where
gradient <- makeRadialGradient c dpr p1 rad1 rad2 color1 color2
VG.strokePaint c gradient

setStrokeBoxGradient rect rad feather color1 color2 = do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this for completeness, but it is not used inside Monomer, so could be removed.

Copy link
Owner

@fjvallarino fjvallarino Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to have it.

@@ -65,6 +65,7 @@ data BaseThemeColors = BaseThemeColors {
dialogText :: Color,
dialogTitleText :: Color,
emptyOverlay :: Color,
shadow :: Color,
Copy link
Owner

@fjvallarino fjvallarino Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this type have some prefix on its fields, since they're all exported and may cause conflicts? It's something I've had in my mind for a while, but I've always postponed it.

Something like baseTheme...:

    ...
    baseThemeEmptyOverlay :: Color,
    baseThemeShadow :: Color
    ...

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A prefix (and then some lenses) seems like it would be consistent with other record types I have seen in the Monomer codebase.

But no strong (or particularly educated) opinions here either way to be honest!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I'll add prefix/lenses in a follow-up PR. Thanks for the recommendation!

@fjvallarino
Copy link
Owner

@Dretch the PR looks great!

My only question is if there's any configuration attribute to select the orientation of the shadow (based on the attached images). Thanks!

@Dretch
Copy link
Contributor Author

Dretch commented Sep 20, 2022

@fjvallarino

My only question is if there's any configuration attribute to select the orientation of the shadow (based on the attached images).

I suppose this might be useful.

Perhaps the shadow location could be specified with a pair of fields like (AlignV, AlignH) - with the default (MacOS) style being (ABottom, ACenter), Android being (ABottom, ARight) and the original proposal being (AMiddle, ACenter)?

Then, should these fields be config options on BoxShadow, or ThemeState fields next to _thsShadowColor (so they apply to everything automatically)?

E.g.

data ThemeState = ThemeState {
  _thsEmptyOverlayStyle :: StyleState,
  _thsShadowColor :: Color,
  _thsShadowAlignV :: AlignV,
  _thsShadowAlignH :: AlignH,
  _thsBtnStyle :: StyleState,
...

@fjvallarino
Copy link
Owner

@Dretch I usually avoid exposing AlignH/AlignV in configuration options, using instead the CmbAlignLeft, etc combinators. This way users can do:

boxShadow_ [alignRight, alignBottom] $
  ...

For the internal representation it makes sense to use AlignH/AlignV as you suggest. The box and popup components follow this pattern. In both cases, if conflicting options are added (alignLeft and then alignRight), the last one takes precedence.

I think having a default in the theme and the ability to customize it in the widget itself would be ideal.

@Dretch
Copy link
Contributor Author

Dretch commented Sep 29, 2022

Hi @fjvallarino , I have pushed a commit to allow configuring the shadow location via theme or config, hopefully in the way you were thinking.

@fjvallarino
Copy link
Owner

@Dretch looks great! Thanks!

@fjvallarino fjvallarino merged commit 38391f7 into fjvallarino:main Oct 2, 2022
@Dretch Dretch deleted the alert-shadows branch October 8, 2022 10:55
@Dretch
Copy link
Contributor Author

Dretch commented Oct 8, 2022

Thanks @fjvallarino ,

Would it be possible for you to do a new release of Monomer?

I'd like this mainly because I want to release https://github.com/Dretch/monomer-hagrid to Hackage and (because I used the new dot-record-syntax stuff) it needs GHC 9.2.x, which means it needs e7ea45f

@fjvallarino
Copy link
Owner

@Dretch released!

I'm still waiting for a new Stackage LTS for GHC 9.2. I've been trying the nightlies and they work well, even fixing issue #1 (although I will need to change the Setup page to recommend installing Stack with ghcup).

@Dretch
Copy link
Contributor Author

Dretch commented Oct 10, 2022

Thanks @fjvallarino 🚢

@fjvallarino
Copy link
Owner

@Dretch have you released Hagrid? Would it be ok if I mention it in Monomer's Readme? Thanks!

@Dretch
Copy link
Contributor Author

Dretch commented Oct 19, 2022

@fjvallarino I have released it to Hackage, yes (though not Stackage, until I get around to that).

Feel free to mention it in the readme 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants