Skip to content

Change shadow color on dark tooltip#208

Merged
VLuisa merged 5 commits intomainfrom
studio-579
Apr 4, 2023
Merged

Change shadow color on dark tooltip#208
VLuisa merged 5 commits intomainfrom
studio-579

Conversation

@VLuisa
Copy link
Copy Markdown
Contributor

@VLuisa VLuisa commented Mar 31, 2023

Added a light shadow to the dark tooltip to improve visibility against dark backgrounds:
image

Also updated all tooltip shadows to use drop-shadow for more accurate shading.

@tristen for approval

@VLuisa VLuisa requested a review from tristen March 31, 2023 18:12
@tristen
Copy link
Copy Markdown
Member

tristen commented Mar 31, 2023

Ah! @VLuisa one thought: is it possible to use a stroke attribute on the arrow element?

<TooltipPrimitive.Arrow width={12} height={6} offset={6} fill={fill} />

I wonder if that might achieve the clean outline around the whole tooltip as you had proposed.

@VLuisa
Copy link
Copy Markdown
Contributor Author

VLuisa commented Mar 31, 2023

@tristen Good point! If I'm understanding correctly, I think that only gets it partway there? Because then each item has it's own stroke (illustrated with red here) as opposed to having one trace all the way around (which can be done but kind of in a hacky way which I'm not sure is worth it?)
image

@tristen
Copy link
Copy Markdown
Member

tristen commented Apr 1, 2023

Ah! @VLuisa thanks for trying that out! I wish stroke accepted positional color values 😞 I can almost achieve it using the drop-shadow filter property but it looks a little funky close-up.

Screenshot 2023-04-01 at 11 00 14 AM

Here's the CSS for that

{ 
  filter: drop-shadow(0 1px 0 red) drop-shadow(0 -1px 0 red) drop-shadow(1px 0 0 red) drop-shadow(-1px 0 0 red)
}

@VLuisa
Copy link
Copy Markdown
Contributor Author

VLuisa commented Apr 3, 2023

@tristen Ooh nice! This feels like it may work because it'd never be observed as closely as in a large screenshot?
Just to confirm the workflow, would this require adding a new CSS class in assembly?

@tristen
Copy link
Copy Markdown
Member

tristen commented Apr 3, 2023

Just to confirm the workflow, would this require adding a new CSS class in assembly?

I think this can be added as inline CSS because it's pretty specific to this option and doesn't require pseudo-selectors. Something like this?

import variables from '@mapbox/mbx-assembly/dist/variables.json';

return (
  <div style={{
    filter:  `
      drop-shadow(0 1px 0 ${variables['--gray-deep']})
      drop-shadow(0 -1px 0 ${variables['--gray-deep']})
      drop-shadow(1px 0 0 ${variables['--gray-deep']})
      drop-shadow(-1px 0 0 ${variables['--gray-deep']})
    `
  />
);

@VLuisa
Copy link
Copy Markdown
Contributor Author

VLuisa commented Apr 4, 2023

@tristen This (mostly) worked for the dark tooltip - as you said, if you don't look too closely.
image

So naturally, I tried applying it to all the tooltips and... it doesn't look great 😅
image
image

I'm leaning towards keeping the dark tooltip as it is in this PR so that they're all consistent. But we could also use this outline styling for just the dark tooltip - curious what you think!

@VLuisa
Copy link
Copy Markdown
Contributor Author

VLuisa commented Apr 4, 2023

@tristen can't re-request review through the github UI, but I made our proposed changes using drop-shadow!

Copy link
Copy Markdown
Member

@tristen tristen left a comment

Choose a reason for hiding this comment

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

I think this looks great!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice way to test this!

@VLuisa VLuisa changed the title Change border color on dark tooltip Change shadow color on dark tooltip Apr 4, 2023
@VLuisa VLuisa merged commit 6d5d2b8 into main Apr 4, 2023
@VLuisa VLuisa deleted the studio-579 branch April 4, 2023 16:41
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