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

fix(3000): Improve opacity of profile pictures #19004

Merged
merged 24 commits into from
Dec 6, 2023

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Nov 30, 2023

Problem

This looked wrong:

Changes

This looks right:

(Though, because of the background being transparent if there's a gravatar, there is now a flash of purple on app load.)

@Twixes Twixes requested a review from a team November 30, 2023 21:32
Copy link
Contributor

github-actions bot commented Nov 30, 2023

Size Change: 0 B

Total Size: 1.83 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.83 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

35 snapshot changes in total. 0 added, 35 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

30 snapshot changes in total. 0 added, 30 modified, 0 deleted:

  • chromium: 0 added, 30 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@daibhin
Copy link
Contributor

daibhin commented Dec 1, 2023

There's a lot of button changes that need to be merged in this PR: #18949. Worried we'll have unnecessary merge conflicts if we make these changes on the old style buttons. I'm happy to work these changes into the new PR or hold off until after it has merged and do them as a follow up

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

9 snapshot changes in total. 0 added, 9 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

9 snapshot changes in total. 0 added, 9 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes requested a review from daibhin December 5, 2023 18:59
@Twixes
Copy link
Member Author

Twixes commented Dec 5, 2023

Re-requesting your review @daibhin since I want to be sure I did styles correctly in the new buttons!

padding: 0;
margin-left: 0.25rem;
}
}

.LemonButton--small .LemonButtonWithSideAction__spacer--divider {
height: 1.125rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe beyond the scope of this PR but feels like this value could drift from https://github.com/PostHog/posthog/pull/19004/files#diff-c694fbbe6b6ff7273c964743f37751e5416009c150d0a9ddad1b4c031ef006d3R264. Perhaps the height should be a calc based on the button height minus some vertical padding

Copy link
Member Author

Choose a reason for hiding this comment

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

This actually shouldn't be needed with the recent refactoring, so removed!

@@ -300,6 +313,11 @@
}
}

.LemonButton__icon {
color: currentColor;
opacity: 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does --lemon-button-icon-opacity: 0.5 work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! Thanks, changed to that

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes force-pushed the improve-3000-profile-picture-opacity branch from 549a5ac to fcec097 Compare December 6, 2023 13:05
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes force-pushed the improve-3000-profile-picture-opacity branch from e88daca to cb2a332 Compare December 6, 2023 13:28
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm thought I fixed this in #19045

Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen this flapping on a bunch of PRs now

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes merged commit 7e2065a into master Dec 6, 2023
78 checks passed
@Twixes Twixes deleted the improve-3000-profile-picture-opacity branch December 6, 2023 19:10
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.

4 participants