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.

Sign up for GitHub

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

Jump to bottom

Fixed icon shadows #359

Merged
merged 4 commits into from
Oct 13, 2024
Merged

Fixed icon shadows #359

merged 4 commits into from
Oct 13, 2024

Conversation

lopatoj
Copy link
Contributor

@lopatoj lopatoj commented Jul 3, 2023

No description provided.

@stsdc
Copy link
Member

stsdc commented Jul 3, 2023 •
edited
Loading

Hi @lopatoj ! Can you elaborate a bit on this change?

@stsdc
Copy link
Member

stsdc commented Jul 30, 2023

@lopatoj ping

@stsdc
Copy link
Member

stsdc commented Oct 9, 2024

@danirabbit can you tell the difference here?

@ryonakano
Copy link
Member

can you tell the difference here?

The bottom portion seems to get subtly brighten:

Before:
Screenshot from 2024-10-11 22-26-00

After:
Screenshot from 2024-10-11 22-26-05

@stsdc
Copy link
Member

stsdc commented Oct 11, 2024

The question is: Should I merge this?
Is this in line with the overall icon style system?

@ryonakano
Copy link
Member

The question is: Should I merge this?
Is this in line with the overall icon style system?

Yeah that's the next question, but I'm not sure because I'm not familiar with elementary HIG.
Any opinions from @elementary/ux?

@ryonakano ryonakano requested a review from a team October 12, 2024 01:59
wpkelso
wpkelso approved these changes Oct 13, 2024
View reviewed changes
Copy link
Member

@wpkelso wpkelso left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You should be good to merge this

In the old version, the icon's drop shadow is sitting on top of the icon itself, while in the new one it's been pushed down the z-index to tuck under the icon. The shadow no longer being on top is whats causing the brightening :)

@stsdc stsdc merged commit 71b9865 into elementary:dev Oct 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stsdc stsdc stsdc approved these changes

@wpkelso wpkelso wpkelso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lopatoj @stsdc @ryonakano @wpkelso