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: button destroy states in 3000 #19086

Merged
merged 13 commits into from
Dec 5, 2023
Merged

fix: button destroy states in 3000 #19086

merged 13 commits into from
Dec 5, 2023

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Dec 5, 2023

Problem

We do not support the type=primary & status=danger combo in PostHog 3000 (yet... until we decide on the new button system)

Changes

  • Replace any instance of danger buttons where the type is primary with type="secondary" only in 3000
Before After
Screenshot 2023-12-05 at 11 16 49 Screenshot 2023-12-05 at 11 18 14
Before After
Screenshot 2023-12-05 at 11 22 12 Screenshot 2023-12-05 at 11 23 22

Needed to change one place we had two danger buttons next to each other because we no longer have a primary & secondary danger button. Decided to use tertiary instead, for now

Before After
Screenshot 2023-12-05 at 11 27 55 Screenshot 2023-12-05 at 11 34 51

How did you test this code?

By 👁️

@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.

@@ -78,7 +80,7 @@ export function AppManagementView({
}
data-attr="plugin-uninstall"
>
Uninstall
Uninstall me
Copy link
Contributor

Choose a reason for hiding this comment

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

why the "me", in any case this is the older UI that will be replaced by pipeline folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! That was just me testing to make sure I was looking at the correct button. Removed in the latest commit

Copy link
Contributor

github-actions bot commented Dec 5, 2023

Size Change: +237 B (0%)

Total Size: 1.83 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.83 MB +237 B (0%)

compressed-size-action

@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 daibhin force-pushed the dn-fix/button-states branch from 283b3a9 to 14bdb74 Compare December 5, 2023 14:09
Copy link
Contributor

@thmsobrmlr thmsobrmlr left a comment

Choose a reason for hiding this comment

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

The hook needs to be adapted to the now multivariate feature flag. Otherwise this looks good.

frontend/src/lib/components/NotFound/index.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/persons/PersonDeleteModal.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/pipeline/AppsManagement.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/settings/project/ProjectDangerZone.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/settings/user/PersonalAPIKeys.tsx Outdated Show resolved Hide resolved
@daibhin daibhin requested a review from thmsobrmlr December 5, 2023 16:20
@daibhin daibhin merged commit 82f3ff4 into master Dec 5, 2023
78 checks passed
@daibhin daibhin deleted the dn-fix/button-states branch December 5, 2023 17:15
Twixes pushed a commit that referenced this pull request Dec 6, 2023
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