-
Notifications
You must be signed in to change notification settings - Fork 61
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
Added output data changed signal #1006
Added output data changed signal #1006
Conversation
0a80768
to
2f7993a
Compare
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 contribution. I have some comments.
@@ -143,6 +144,9 @@ export class SignalManager extends EventEmitter implements SignalManager { | |||
fireRedoSignal(): void { | |||
this.emit(Signals.REDO); | |||
} | |||
fireOutputDataChanged(): void { | |||
this.emit(Signals.OUTPUT_DATA_CHANGED); |
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.
Should the signal have a payload to indicate which data provider or data providers were changed? This would allow receivers to filter out signals and only act on it if it is meant for them?
WDYT?
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.
Yeah that is a good idea. I will add that in the next patch.
packages/react-components/src/components/utils/timegraph-container-component.tsx
Show resolved
Hide resolved
@ngondalia Interesting update. It sounds like that it is for some streaming / monitoring use case. If your server is modifying data, do you have some notification channel implemented from your custom server to the front-end? Please let me know. |
Sorry, the word "dynamically" was misleading and wrong here. The data gets modified based on some trace type specific settings/filters coming from our custom webviews within our extension. The front end initiates the data modification, therefore we dont have the need for the server to notify the front-end of anything. It already knows that data has changed, as well as the type of data that has changed. |
Introduced a new signal to indicate that the data on the server has been updated and therefore the components should refetch the data and rerender the components. This PR handles the signal in table-output-component and timegraph-output-component. We have a use case where data is dynamically modified on the server, and we want the components to rerender with the new data. Currently, the user would have to close the active trace panel and reopen it to see the changes. Added output descriptors as the signal payload. Signed-off-by: Neel Gondalia <ngondalia@blackberry.com>
2f7993a
to
63c0789
Compare
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 looks good to me. To test I added a command to trigger the refresh. Thanks for the contribution.
Introduced a new signal to indicate that the data on the server has been updated and therefore the components should refetch the data and rerender the components. This PR handles the signal in table-output-component and timegraph-output-component. We have a use case where data is modified on the server, and we want the components to rerender with the new data. Currently, the user would have to close the active trace panel and reopen it to see the changes.