-
Notifications
You must be signed in to change notification settings - Fork 296
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
Rares/webhook UI changes #2419
Rares/webhook UI changes #2419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about adding some basic e2e tests for the new webhooks? ex. ability to CRUD them + webhook is called when when we would expect (for this one you'd have to run ngrok
or localtunnel
to port forward a public URL to a server running on the tests; I can help out with this if you're interested)
!outgoingWebhook2Store.items[id] | ||
) { | ||
return null; | ||
} | ||
|
||
const data = | ||
id === 'new' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's confusing a bit, what the difference between action === WebhookFormActionType.NEW
and id === 'new'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'll refactor it
(action === WebhookFormActionType.EDIT_SETTINGS || action === WebhookFormActionType.VIEW_LAST_RUN) && | ||
!outgoingWebhook2Store.items[id] | ||
) { | ||
// nothing to show if we open invalid ID for edit/last_run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it be unloaded in store yet at the moment this code is running?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by this. This is when you're hitting an ID that was previously deleted or it just doesn't exist. There's no such object in the store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I understand we don't show anything if there is no such item in the store, but in this case we need to force load it within this component
} | ||
|
||
if (action === WebhookFormActionType.NEW || action === WebhookFormActionType.COPY) { | ||
// show just the creation form, not the tabs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mabe just make LastRun tab disabled for those cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows the figma where on new/copy there's no tabs shown, plus the content is slightly different.
import { useStore } from 'state/useStore'; | ||
import { KeyValuePair } from 'utils'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what the pros of using this data structure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a key-value structure for when you need to keep groups of key/value pairs. It's used in other places as well.
{({ openMenu }) => ( | ||
<HamburgerMenu openMenu={openMenu} listBorder={ACTIONS_LIST_BORDER} listWidth={ACTIONS_LIST_WIDTH} /> | ||
)} | ||
{({ openMenu }) => <HamburgerMenu openMenu={openMenu} listBorder={2} listWidth={200} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be precise it's a kebab (see https://alvarotrigo.com/blog/assets/imgs/2021-12-14/hamburger-menu-types-examples.webp) =))
Will postpone adding the tests as I need to help Maxim add the template editor for webhooks. |
# What this PR does ## Which issue(s) this PR fixes ## Checklist - [ ] Unit, integration, and e2e (if applicable) tests updated - [ ] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
What this PR does
Which issue(s) this PR fixes
Checklist
pr:no public docs
PR label added if not required)CHANGELOG.md
updated (orpr:no changelog
PR label added if not required)