-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[NIFI-12754] - Flow Configuration History #8399
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.
Thanks for the PR @rfellows! Looking great so far. I really like the time inputs. Thanks for updating that in the Provenance search too. I did notice a few minor things on first-pass that I've detailed below.
...i/src/app/pages/flow-configuration-history/feature/flow-configuration-history.component.html
Show resolved
Hide resolved
...rc/app/pages/flow-configuration-history/feature/flow-configuration-history-routing.module.ts
Outdated
Show resolved
Hide resolved
...tory/ui/flow-configuration-history-listing/flow-configuration-history-listing.component.html
Show resolved
Hide resolved
...story/state/flow-configuration-history-listing/flow-configuration-history-listing.effects.ts
Outdated
Show resolved
Hide resolved
...story-listing/flow-configuration-history-table/flow-configuration-history-table.component.ts
Outdated
Show resolved
Hide resolved
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.
Thanks for the updates @rfellows! Just a couple minor things I noticed on this pass.
...istory/ui/flow-configuration-history-listing/flow-configuration-history-listing.component.ts
Outdated
Show resolved
Hide resolved
styleUrls: ['./flow-configuration-history-table.component.scss'] | ||
}) | ||
export class FlowConfigurationHistoryTable { | ||
@Input() selectedHistoryActionId: number | null = 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.
Does this need to be an @Input
and saved in the state. We have a few other cases of table selection scenarios where we are not deep linking and the component just internally manages the selected row.
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 doesn't "need" to be. But it was a simple enough change to migrate from using the selection from the route to this. And it sets us up to change back to use the route for selection in the future if we choose.
...tory/ui/flow-configuration-history-listing/flow-configuration-history-listing.component.scss
Outdated
Show resolved
Hide resolved
* support selection * support pagination * support sorting * added time controls, also updated provenance to use them too * allow for clearing of filters * use date range filter * more details dialog for flow config history * support purge history
…een header and page content
e68583a
to
86e10dd
Compare
…one to purge confirmation message * reset pagination state on filter, clear filter, and purge * only resubmit query when filter by changes IF there is a filter term specified
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.
Thanks for the updates @rfellows! Looks good. +1 will merge once CI completes.
NIFI-12754