-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature/2826 create feed source UI #268
Conversation
…ing in FeedSourceManager
…edSourceForm props and templates
setDynamicFormElement(null); | ||
} | ||
} | ||
}, [formStateObject || null]); |
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 is this?
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.
you already have one useEffect for formStateObject...
src/translations/da/common.json
Outdated
"load-feed-source-error": "Der skete en fejl da feed source med følgende id: {{id}} skulle hentes:" | ||
}, | ||
"dynamic-fields": { | ||
"EventDatabaseApiFeedType": { |
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.
translation keys should be lower-case
const NotifiedFeedTypeTemplate = ({ | ||
handleInput, | ||
formStateObject, | ||
t, |
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.
no need to inject t through props...
const { t } = useTranslation("common", { keyPrefix: "notified-feed-type" });
<FormInput | ||
name="host" | ||
type="text" | ||
label={t("dynamic-fields.EventDatabaseApiFeedType.host")} |
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.
see comment above, this will allow you to do:
label={t("host")}
instead of all the dots
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.
Uh, nice!
const EventDatabaseApiTemplate = ({ | ||
handleInput, | ||
formStateObject, | ||
t, |
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.
no need to inject t through props...
const { t } = useTranslation("common", { keyPrefix: "event-database-feed-type" });
} | ||
formStateObject.secrets = newSecrets; | ||
} | ||
}, [formStateObject]); |
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 think this code should only run if formStateObject.feedType changes. This should be achievable by something along the lines of:
useEffect(() => {
if (formStateObject?.feedType) {
... <update secrets>
}
}, [formStateObject.feedType]);
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 revised this part a bit, but since the formstateobject doesn't support nested keys, this useEffect needs to be called on formstateobject updates, to set e.g.
formStateObject.token
into
formStateObject.secrets.token
if (formStateObject && formStateObject.feedType) { | ||
let newSecrets = {}; | ||
|
||
switch (formStateObject.feedType) { |
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 don't think I understand the purpose of this switch...
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.
Refactored
Adjustments
Link to ticket
2826
Description
This PR introduces a user interface for viewing, creating, updating and deleting feed sources.
Screenshot of the result
Checklist
If your code does not pass all the requirements on the checklist you have to add a comment explaining why this change
should be exempt from the list.
Additional comments or questions
If you have any further comments or questions for the reviewer please add them here.