-
Notifications
You must be signed in to change notification settings - Fork 16
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
GDB-11318 implement global notification component #1882
base: GDB-10373-workbench-with-single-spa
Are you sure you want to change the base?
GDB-11318 implement global notification component #1882
Conversation
5da448d
to
03ea737
Compare
f5f29d7
to
ef8e8ec
Compare
## What Implement global notification component ## Why To be able to display statuses of different ongoing operations ## How - Created `onto-operations-notification` dropdown button, which displays different operations as icons and respective count. The button is a dropdown toggle, so when clicked, it opens a list of different operations. When an individual operation is clicked, it redirects to a page, related to the operation - Refactored `onto-header` to display/hide the new notification component and poll the operations on every 2 seconds - Migrated `fibonacci-generator`, which is currently used to slow down requests, if errors occur - Added models and services ## Testing jest and cypress GDB-11839 Migrate font awesome stylesheets ## What Migrate font awesome styles to the new workbench ## Why To have them available at a global level throughout the application ## How - Copied the css and font files for font awesome in `root-config` package - Imported them in root-config via `ontotext-vendor.js` - Removed the imports of these files in the legacy workbench - Removed the NPM dependency to font awesome in root-config and shared-components
ef8e8ec
to
ab864a3
Compare
|
||
constructor(data: OperationGroupSummary) { | ||
super(); | ||
this.id = window.crypto.randomUUID(); |
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 prefer if we don't depend depend on the window directly. Can you create some service or utility and move this random generator there?
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.
Can the service/util use window.crypto.randomUUID()
internally, or should I implement something?
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.
Yes. Just beware that our services are singletons and we don't want to cache the window inside.
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.
moved to generator-utils
}) | ||
}); | ||
|
||
const assertGroupCount = (index, textValue) => { |
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.
Move this outside of the describe block for better visibility or in the steps
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.
moved
private readonly fibonacciGenerator = new FibonacciGenerator(); | ||
|
||
private repositoryId?: string; | ||
private pollingInterval: NodeJS.Timeout; |
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'm not sure if this is the correct type which should be used. In a browser environment setInterval returns a number.
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.
added number
type. Also added window.setInterval
to ensure typescript knows it's a browser interval and not a nodejs. If you don't like depending directly on window here, the other option is //@ts-ignore, since typescript doesn't know, which interval it is.
disconnectedCallback(): void { | ||
this.subscriptions.unsubscribeAll(); | ||
private startOperationPolling() { | ||
this.pollingInterval = setInterval(() => { |
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.
As far as th polling can be restarted in certain conditions, double check if there should be some clearing/reset before starting a new one.
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.
The legacy workbench currently starts polling, when the component is loaded, without clearing the interval beforehand. The interval is cleared upon $destroy
.
The current implementation will stop polling, if the repositoryContext sends repositoryId: undefined
as well. Otherwise it will start and stop, like the legacy
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.
added clearInterval
at the start just in case
...red-components/src/components/onto-operations-notification/onto-operations-notification.scss
Show resolved
Hide resolved
|
||
|
||
private toOperationsGroupSummary() { | ||
const groupToOperationSummary = new Map<OperationGroup, OperationGroupSummary>(); |
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.
Is it possible this mapping building to be moved in the activeOperations model maybe. Better avoid exposing the model internals in components to reduce the coupling
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.
moved inside the model
629202f
to
ecf5530
Compare
- Moved `toOperationsStatusSummary` inside `OperationsStatusSummary` model - Created `generator-utils` and moved `window.crypto.randomUUID` there - Moved `assertGroupCount` outside describe block - replaced `setInterval` with `window.setInterval` so typescript is aware, that it is a browser interval and not a nodejs one - Refactored main.js for the operations-notification page, to accommodate the changes
ecf5530
to
6e21f1c
Compare
Jenkins retry build |
}); | ||
|
||
const operationGroupArray = Array.from(groupToOperationSummary.values()); | ||
return MapperProvider.get(OperationGroupSummaryListMapper).mapToModel(operationGroupArray); |
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.
OK, but now you put a service dependency in the model. Although it is a mapper, I don't think it's a good idea. You can return the array from this method and move this mapping in the client component.
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 I limit mapper usage only to constructors? I've used list mappers in other models and I'm not sure, where to draw the line
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.
Models should not have dependencies than other models. If you've used mappers in models, then those should also be refactored.
|
What
Implement global notification component
Why
To be able to display statuses of different ongoing operations
How
Created
onto-operations-notification
dropdown button, which displays different operations as icons and respective count. The button is a dropdown toggle, so when clicked, it opens a list of different operations. When an individual operation is clicked, it redirects to a page, related to the operationRefactored
onto-header
to display/hide the new notification component and poll the operations on every 2 secondsMigrated
fibonacci-generator
, which is currently used to slow down requests, if errors occurAdded models and services
Testing
jest and cypress
Screenshots
Checklist