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

Fixes the Delete and Update webhook dialogs #1350

Merged
merged 11 commits into from
Oct 26, 2022

Conversation

clacladev
Copy link
Contributor

@clacladev clacladev commented Oct 19, 2022

What does this pull request do? Explain your changes. (required)
Fixes the Delete and Update webhook dialogs, which were displayed behind the body of the content. Fixed the Event selector inside the Create/Update web hook dialog.

Specific updates (required)

  • Displaying the Delete and Update webhook dialogs in front of the content
  • Refactored the page breaking it down into smaller more manageable components
  • Fixed the Events Select to make it work on all browsers (it was working only on Chrome!)
  • Fixed the position of the Events dropdown to not overlap the Select box

How did you test each of these updates (required)
Tested on different browser and dark/light mode.

Does this pull request close any open issues?
Fixes #1340

Screenshots (optional):
Screenshot 2022-10-19 at 12 25 38

Screenshot 2022-10-19 at 12 25 43

Events Select all 3 major browsers:
Screenshot 2022-10-20 at 10 47 22
Screenshot 2022-10-20 at 10 47 52
Screenshot 2022-10-20 at 10 48 20

@clacladev clacladev requested a review from a team as a code owner October 19, 2022 17:26
@vercel
Copy link

vercel bot commented Oct 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
livepeer-studio ✅ Ready (Inspect) Visit Preview Oct 26, 2022 at 11:59AM (UTC)

Copy link
Member

@adamsoffer adamsoffer left a comment

Choose a reason for hiding this comment

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

Looks good! I just noticed a few cosmetic issues related to webhooks that we may as well tackle in this issue as well.

  1. Can we change this border from $primary7 to $neutral6

CleanShot 2022-10-20 at 12 19 57@2x

  1. Can we fix this overflow text? Can use an ellipsis...

CleanShot 2022-10-20 at 12 20 19@2x

  1. Can we fix this z-index on dialog overlays so that it covers the sidebar too?
    CleanShot 2022-10-20 at 12 18 30@2x

@clacladev
Copy link
Contributor Author

clacladev commented Oct 20, 2022

@adamsoffer I fixed the border color and the long urls overlapping. Good catch.
Screenshot 2022-10-20 at 11 35 37

Screenshot 2022-10-20 at 11 56 45

I would suggest that I fix the z-index issue in another PR, given that turns out ALL popups in the app have the same issue. Also, it turns out that ALL popups on smaller screens break.
Should I create a new issue and then work on it on a PR, or should I just work on a new PR?

Screenshot 2022-10-20 at 12 00 52

Screenshot 2022-10-20 at 12 01 09

@adamsoffer
Copy link
Member

I would suggest that I fix the z-index issue in another PR, given that turns out ALL popups in the app have the same issue. Also, it turns out that ALL popups on smaller screens break.
Should I create a new issue and then work on it on a PR, or should I just work on a new PR?

Okay yeah in that case let's create a separate issue + PR to fix the dialog. Sounds good 👍

Copy link
Member

@adamsoffer adamsoffer left a comment

Choose a reason for hiding this comment

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

LGTM! Can we just resolve that merge conflict? Then good to merge.

@clacladev clacladev merged commit 03a20d7 into master Oct 26, 2022
@clacladev clacladev deleted the clacladev/webhooks-popup-fix branch October 26, 2022 13:01
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.

UI Issues when configuring webhooks
2 participants