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

Feature/2826 create feed source UI #268

Draft
wants to merge 68 commits into
base: develop
Choose a base branch
from

Conversation

jeppekroghitk
Copy link

@jeppekroghitk jeppekroghitk commented Oct 28, 2024

Link to ticket

2826

Description

This PR introduces a user interface for viewing, creating, updating and deleting feed sources.

Screenshot of the result

image

Checklist

  • My code is covered by test cases.
  • My code passes our test (all our tests).
  • My code passes our static analysis suite.
  • My code passes our continuous integration process.

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.

@tuj tuj added the enhancement New feature or request label Nov 13, 2024
setDynamicFormElement(null);
}
}
}, [formStateObject || null]);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

Copy link
Contributor

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

"load-feed-source-error": "Der skete en fejl da feed source med følgende id: {{id}} skulle hentes:"
},
"dynamic-fields": {
"EventDatabaseApiFeedType": {
Copy link
Contributor

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,
Copy link
Contributor

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")}
Copy link
Contributor

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

Copy link
Author

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,
Copy link
Contributor

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]);
Copy link
Contributor

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]);

Copy link
Author

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) {
Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

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

Refactored

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants