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.

Sign up for GitHub

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

Jump to bottom

Improved event update #396

Merged
merged 2 commits into from
Jun 17, 2024
Merged

Improved event update #396

merged 2 commits into from
Jun 17, 2024

Conversation

LaChope
Copy link
Collaborator

@LaChope LaChope commented Jun 10, 2024

Do not resolve any specific issue, it it a global improvement including:

  • Root events can no longer be updated (read only)
  • External events do not have probability field anymore

@LaChope LaChope requested a review from blcham June 10, 2024 12:15
@LaChope LaChope mentioned this pull request Jun 10, 2024
Merged
@LaChope LaChope removed the request for review from blcham June 10, 2024 12:20
@LaChope LaChope marked this pull request as draft June 10, 2024 12:21
@blcham
Copy link
Contributor

blcham commented Jun 10, 2024

External events do not have probability field anymore

@LaChope can you explain this? They should have probability

@LaChope LaChope marked this pull request as ready for review June 11, 2024 12:42
@LaChope LaChope requested a review from blcham June 11, 2024 12:42
@LaChope LaChope linked an issue Jun 11, 2024 that may be closed by this pull request
created SNS events should not have failure rate #394
Closed
@LaChope
Copy link
Collaborator Author

LaChope commented Jun 11, 2024

@blcham Ediding an event is still not perfect yet, but I think it makes sense to wait for #281 to be completed first.

blcham
blcham requested changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@blcham blcham left a comment

Choose a reason for hiding this comment

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

See my comments.

src/utils/utils.tsx
* @returns {Array<FaultEvent>} A new array of event objects with updated eventType values where applicable.
*
*/
export const updateEventsType = (events: FaultEvent[], substring: string, newValue: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, it is no possible to declare output type here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what you mean by that? something like: const updateEventsType: FaultEvent[] = (events: FaultEvent[], ...)

Copy link
Contributor

@blcham blcham Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure but i would think something like:

(events: FaultEvent[], substring: string, newValue: string)

--->

(events: FaultEvent[], substring: string, newValue: string): FaultEvent[]

Copy link
Contributor

Choose a reason for hiding this comment

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

@LaChope

Copy link
Contributor

Choose a reason for hiding this comment

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

@LaChope

src/components/dialog/faultEvent/FaultEventCreation.tsx Show resolved Hide resolved
src/components/dialog/faultEvent/FaultEventCreation.tsx Show resolved Hide resolved
src/components/dialog/faultEvent/FaultEventCreation.tsx Show resolved Hide resolved
@blcham
Copy link
Contributor

blcham commented Jun 11, 2024 •
edited
Loading

@blcham Ediding an event is still not perfect yet, but I think it makes sense to wait for #281 to be completed first.

I assume that you do not want me to merge before merging #281

@LaChope
Copy link
Collaborator Author

LaChope commented Jun 17, 2024

@blcham @Kasmadei what is the state of #281 ? If it is solved, I will merge this PR after resolving the conversation.

@Kasmadei
Copy link
Collaborator

@LaChope
Hi, i think you can merge. Task is almost done (I have a few changes to merge locally), main part was merged.

@LaChope
Copy link
Collaborator Author

LaChope commented Jun 17, 2024

@blcham After merge, the UI does not really look great anymore (see screenshot below), should I still merge and create new ticket for it or should I continue here?

image

@LaChope LaChope merged commit 172b1fd into main Jun 17, 2024
1 check passed
@LaChope LaChope deleted the improve-event-update branch June 17, 2024 12:18
@LaChope LaChope mentioned this pull request Jun 17, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blcham blcham blcham requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

created SNS events should not have failure rate
3 participants
@LaChope @blcham @Kasmadei