-
Notifications
You must be signed in to change notification settings - Fork 53
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
PTV-1905 - add service widget support #3421
Open
eapearson
wants to merge
107
commits into
develop
Choose a base branch
from
PTV-1905
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 103 commits
Commits
Show all changes
107 commits
Select commit
Hold shift + click to select a range
461d44c
add preact and htm dependencies [PTV-1905]
eapearson cfa05a7
add some support components [PTV-1905]
eapearson 802adcd
improve by adding support for modal size and the "shown" event [PTV-1…
eapearson df3f3f7
adds service widget cell [PTV-1905]
eapearson 7eda4b1
add developer menu for running service widgets [PTV-1905]
eapearson 0c27157
add support for dynamic service widget output for apps [PTV-1905]
eapearson 55e4afe
add support for service widget data viewers [PTV-1905]
eapearson f123af0
shorten widget name [PTV-1905]
eapearson c7f2d06
improve title and add subtitle support [PTV-1905]
eapearson 232509f
improve title and subtitle support [PTV-1905]
eapearson 5112f2f
remove space added by formatter [PTV-1905]
eapearson 3f5d6ff
fix generic add service widget form [PTV-1905]
eapearson 6e0bc32
add "er" to "develop" in menu [PTV-1905]
eapearson 1665de9
add developer conveniences to celltoolbar and cell expand/collapse [P…
eapearson 783635c
improve feature detection, add dynamic feature enablement [PTV-1905]
eapearson 7f620ff
remove previously used and now unused code [PTV-1905]
eapearson 13a4ba5
fix indentation for generated Python [PTV-1905]
eapearson b5afc4b
move direct styles to stylesheet [PTV-1905]
eapearson 5d7ad42
add app output icon which uses stacked style, fix stacked style [PTV…
eapearson 171667f
bunch of improvements to service widget [PTV-1905]
eapearson 76f3d83
clean up code, fix channel window [PTV-1905]
eapearson 721ed71
improve app output icons [PTV-1905]
eapearson 9eb2e22
fix cases of double click on menu items toggling min/max of cell [PTV…
eapearson 6ee5b51
improve constant naming [PTV-1905]
eapearson 063430d
undo autoformatting [PTV-1905]
eapearson 04f0435
move some css to scss, refactor dialogs to preact and extract the tab…
eapearson 1648f53
fix linting errors [PTV-1905]
eapearson bbf5fc2
filter out developer tools in view-only mode [PTV-1905]
eapearson 6423967
document, refactor, simplify [PTV-1905]
eapearson bf6fbbc
fix toolbar debounce, clean up window listener when cell is deleted […
eapearson 0e6d647
safer method of getting app panel tag, remove testing code for viewer…
eapearson ea3ef4b
remove indenting fix code (breaks tests, not work refactoring to fix)…
eapearson 024929f
add release note [PTV-1905]
eapearson bce9e0e
add service widget cell documentation (wip) [PTV-1905]
eapearson 079c717
disable usage of Narrative-wide userSetting [PTV-1905]
eapearson 52e3ca9
use uuid [PTV-1905]
eapearson f7ab4b5
revert variable name change; small edits for PR sonarcloud [PTV-1905]
eapearson ac02ed5
revert some code clarity refactorings [PTV-1905]
eapearson 49198b6
undo empty line removal [PTV-1905]
eapearson e5c8b22
missed this one [PTV-1905]
eapearson 14cc511
move styles into scss [PTV-1905]
eapearson c4f84cf
remove "cell" parameter; not needed as is set on the object [PTV-1905]
eapearson 4825569
return empty string rather than undefined [PTV-1905]
eapearson 30ffe34
rename [PTV-1905]
eapearson c946207
rename [PTV-1905]
eapearson c7ebe7d
trying to make git rebase happy with this renamed file [PTV-1905]
eapearson 6407fdc
more renaming [PTV-1905]
eapearson e650254
add README.md [PTV-1905]
eapearson dfae3cf
include cellMangar in module exports, to avoid useless assignment err…
eapearson 8132ed2
remove extra blank line [PTV-1905]
eapearson ff5c0dd
add preact components to exclusion list, add tests for common preact …
eapearson a7d7992
fix something i broke, and was revealed by tests [PTV-1905]
eapearson f1f480d
temporarily disable this test, needs more work [PTV-1905]
eapearson b3e361a
regenerate package-lock.json [PTV-1905]
eapearson a1d63c1
package.json and lock file updated [PTV-1905]
eapearson cd0531d
rebuild all_concat.css and all_concat.css.map [PTV-1905]
eapearson b09acab
fix "AddServiceWidget" dev tool [PTV-1905]
eapearson 6da9499
don't use spread syntax, esprima doesn't like it [PTV-1905]
eapearson 53d1632
convert more object spreads to Object.assign [PTV-1905]
eapearson 9051a4a
more object spread refactor [PTV-1905]
eapearson b803f7a
hoping that is all the object spread cases [PTV-1905]
eapearson bda7d89
accept an IMAGE env variable, defaulting to current behavior [PTV-1905]
eapearson 4ae7379
Add developer mode toggle in hamburger menu [PTV-1905]
eapearson 6df3162
restore previous behavior [PTV-1905]
eapearson 8fb4495
remove isDeveloper(), isAdvanced() from runtime.js [PTV-1905]
eapearson 446d586
use "code" icon for developer menu item [PTV-1905]
eapearson c868a8f
improve title for generic service widget form [PTV-1905]
eapearson f038d1e
remove padding from dialog footer if no buttons [PTV-1905]
eapearson 38a8ac7
autofocus first form field [PTV-1905]
eapearson 56051df
Merge remote-tracking branch 'origin/develop' into PTV-1905
eapearson 8d79c70
improve url feature detection [PTV-1905]
eapearson 578f85b
make sonarcloud happier [PTV-1905]
eapearson 969df12
make sonarcloud happier [PTV-1905]
eapearson 55218fc
add alt attribute [PTV-1905]
eapearson 4e24693
use startsWith rather than charAt [PTV-1905]
eapearson 625b8df
disable warning on object.assign [PTV-1905]
eapearson c8549fe
remove commented out code [PTV-1905]
eapearson f57e1c8
fix type in jsdoc [PTV-1905]
eapearson 013b2d3
avoid unused variables [PTV-1905]
eapearson ea99deb
try NOSONAR on same line [PTV-1905]
eapearson 00dd3cf
try NOSONAR again [PTV-1905]
eapearson d87c421
remove unnecessary eslint-disable [PTV-1905]
eapearson 4c5fb0d
condense boolean expression [PTV-1905]
eapearson faa8d7d
improve param validation [PTV-1905]
eapearson 3da0859
simplify conditional expression [PTV-1905]
eapearson a397cac
remove TODO [PTV-1905]
eapearson d52da2c
remove TODO [PTV-1905]
eapearson 845deb0
remove a few TODOs [PTV-1905]
eapearson 851ddff
Merge remote-tracking branch 'origin/develop' into PTV-1905
eapearson 40aeef7
add prop-types support, and usage in ErrorAlert [PTV-1905]
eapearson 6bee246
add narrative_paths to local docker volume mounts [PTV-1905]
eapearson a5c36b9
style tweaks [PTV-1905]
eapearson 2669db4
working on prop-types [PTV-1905]
eapearson 080125d
added additional prop types [PTV-1905]
eapearson e065ced
more prop-types [PTV-1905]
eapearson 2b61bde
remove empty styles, remove unused stylesheet and stylesheet referenc…
eapearson 636ac7d
remove a few unnecessary TODOs [PTV-1905]
eapearson 85168b0
Merge remote-tracking branch 'origin/develop' into PTV-1905
eapearson 70f83b7
package-lock.json missing htm after merge conflict resolution [PTV-1905]
eapearson 16c97e4
remove unused render local variable [PTV-1905]
eapearson 3e7c88a
replace object assign with object spread [PTV-1905]
eapearson f446dbf
revert that object spread [PTV-1905]
eapearson a6f28ea
Merge remote-tracking branch 'origin/develop' into PTV-1905
eapearson c09cd56
Merge branch 'develop' into PTV-1905
briehl cec97a0
update deps, package-lock
briehl ab96c48
recompile css
briehl 73d74ff
minor typing updates
briehl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
# Dynamic Service Widget Cells | ||
|
||
## Contents | ||
|
||
- about | ||
- [design](design.md) | ||
|
||
## About | ||
|
||
A " service widget" is (at the time of writing) a new kind of user interface | ||
soon to be available within KBase. | ||
|
||
A service widget is composed of two main pieces: | ||
|
||
1. the service widget "web app" served by a dynamic service | ||
2. a type of Narrative cell to fetch and render them. | ||
|
||
A _service widget_ is a single page web app. Even simpler than that - as far as the | ||
Narrative is concerned it is a URL of a specific format that returns `text/html` content. | ||
|
||
The Narrative hosts such "widgets" in a dedicated "service widget Cell" - | ||
`serviceWidget`. The `serviceWidget` cell implements the interface to service widgets, | ||
which includes embedding an iframe which requests and renders the service widget app and a | ||
communication protocol using window messages. | ||
|
||
In the initial implementation (MVP-1) there is little interaction between the widget and the | ||
Narrative. (Previous prototyping work demonstrated two-way interaction between a Narrative and a | ||
service widget. In that case, it was utilized to allow a service widget to have | ||
persistent state, stored in the cell's metadata.) | ||
|
||
Dynamic service widgets are integrated into the Narrative in two ways: | ||
|
||
1. As data viewers - the viewer displayed when a data object is inserted into a Narrative | ||
|
||
2. As app output - the cell displayed when an app completes, and specifies that it wants | ||
to render an output widget | ||
|
||
In addition, a developer tool allows inserting arbitrary service widget cells, which do | ||
not not necessarily adhere to either the the data viewer or app output viewer formats. |
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,300 @@ | ||
# Dynamic Service Widget Cells | ||
|
||
## Contents | ||
|
||
- [about](about.md) | ||
- design | ||
|
||
## Design | ||
|
||
### Overview | ||
|
||
Dynamic service widgets are integrated into the Narrative in three important ways: | ||
|
||
- they are hosted in a new type of cell, `serviceWidget` | ||
- they may be used as data object viewers, with changes to data viewer handling | ||
- they may be used as app output viewers, with changes to app cell output handling | ||
|
||
Overall, the changes are mostly "additive" - adding functionality which does not affect | ||
existing code - with most changes to existing code being conditionalized (protected with | ||
conditional branches). | ||
|
||
Two new external libraries are added; `preact` and `htm` have been added to support a | ||
more familiar (dare we say "modern"?) style of component architecture. | ||
|
||
> This decision can be reversed, and we can rewrite the components in jquery, but the | ||
> initial prototype was based on preact and htm to facilitate rapid development. | ||
|
||
### Changes, Additions | ||
|
||
- New serviceWidget cell implementation | ||
- New cell classes for notebook and cell runtime integration | ||
- Usage of iframe to host external widgets served by dynamic services | ||
- Resizable cell, with persistence | ||
- Hook in data viewer code and viewer config conventions to use service widget as data | ||
viewer | ||
- Hook in app output code and app config conventions to use service widget as app | ||
output viewer | ||
- Development tools to assist those creating or maintaining service widgets in the | ||
Narrative. | ||
|
||
### Service Widget Cell | ||
|
||
The service widget cell is implemented as a notebook extension in | ||
`nbextensions/serviceWidgetCell`. | ||
|
||
The service widget cell has special features: | ||
|
||
- It is based on new `CellManager` and `CellBase` classes; | ||
subclasses implement the specific behavior for the service widget cell | ||
|
||
- Provides a "service lifetime" model (i.e. "start", "stop"), because | ||
service widgets utilize DOM listeners which need to be constructed when they are | ||
started, and torn down when they are stopped and removed. | ||
|
||
- Widget state may be persisted in the Narrative; this feature was prototyped but is | ||
not presently active. | ||
|
||
- Utilizes `preact` and `htm` to enable usage of React-style components to help control | ||
complexity and utilize a more familiar component pattern (compared to jQuery, | ||
kbwidgets, or old kbase-ui style widgets). | ||
|
||
The basic design of the service widget cell is: | ||
|
||
- construct a url for a given widget in a given dynamic service. | ||
- embed this url in an iframe inside the cell's output area | ||
- allow user to resize the output area height, and persist it | ||
|
||
##### CellManager and CellBase | ||
|
||
> TO BE WRITTEN | ||
|
||
#### IFrame Embedding | ||
|
||
A key to the service widget implementation is the usage of an iframe embedded in the | ||
service widget cell. This allows the service widget "web app" to render naturally in the | ||
cell output area. | ||
|
||
We'll cover the constraints that guided this implementation: | ||
|
||
- url format | ||
- authentication | ||
- window message integration | ||
|
||
##### Widget URL | ||
|
||
A widget url is a special form of a dynamic service url. Dynamic service urls were | ||
previously used only for JSON-RPC 1.1 calls (with rare exception). | ||
|
||
The widget URL is used as the target of an iframe (discussed below) embedded in the | ||
cell. | ||
|
||
A widget url looks like this: | ||
|
||
```url | ||
https://ci.kbase.us/dynserv/GITHASH.SDKMODULE/widgets/WIDGETNAME?PARAM1=VALUE1&PARAM2=VALUE2 | ||
``` | ||
|
||
where: | ||
|
||
- `GITHASH` is the git commit hash for a specific build of the dynamic service | ||
- `SDKMODULE` is the module name under which the dynamic service is registered | ||
- `WIDGETNAME` is the name of thw widget within the dynamic service | ||
- All parameters, like `PARAM1` and `PARAM2` above, are named. They are supplied as | ||
search parameters (aka "query parameters", but are officially known as the search | ||
fragment of the url). | ||
|
||
##### Authentication | ||
|
||
Authentication is provided through the existing `kbase_session` and | ||
`kbase_session_backup` cookie. | ||
|
||
The backup cookie is required in production because the service host is `kbase.us`, | ||
while the front end is `narrative.kbase.us`, and a domain cookie (used by Europa) or | ||
host cookie (used by kbase-ui) on `narrative.kbase.us` cannot be accessed on `kbase.us`. | ||
|
||
##### Interprocess communication via window messages | ||
|
||
As for kbase-ui and it's plugins, as for Europa and kbase-ui, the Narrative Interface | ||
runtime via the service widget cell communicates with the service widget web app through | ||
the `postMessage` browser DOM API. | ||
|
||
This communication serves first as a sort of "boot process". We'll go over the entire | ||
service widget cell lifetime below, so we'll just focus on how it works in this section. | ||
|
||
The messaging works through a pair of objects, instances of `SendChannel` and | ||
`ReceiveChannel`. | ||
|
||
Due to the fact that the narrative and a dynamic service providing a widget app may have | ||
different origins, it is not possible to use postMessage bidirectionally on the same | ||
window. In prod, Javascript in the Narrative my not listen for events on the iframe, and | ||
conversely the widget app in the iframe may not listen for events on the Narrative's window. | ||
|
||
Thus, the Narrative service widget cell listens for messages from the widget app on the | ||
Narrative window but sends messages to the widget app in the iFrame window. Conversely, | ||
the widget app in the iFrame window listens for events on the iFrame window and sends | ||
events to the Narrative's window. | ||
|
||
This is managed with a pair of classes. Instances of `SendChannel` are responsible for | ||
sending messages on another window, and instances of `ReceiveChannel` are responsible | ||
for receiving messages in the current window. | ||
|
||
> `Channel` is an abstraction I've used for years to wrap up the process of the window | ||
> `message` event which is used through `addEventListener` and `postMessage`. We can | ||
> choose a different name. | ||
|
||
To sort out messages that may arrive from different sources, two methods are used. | ||
(Other than the built-in target origin of postMessage). | ||
|
||
First, we use a specific message structure. If another process sends a message to a | ||
window, it is highly unlikely that the message format will be the same. | ||
|
||
The structure is | ||
|
||
```javascript | ||
{ | ||
name: 'event_name', // an event name, or identifier | ||
envelope: { | ||
channelId: '6b8e1444-dcc0-42f4-bbcf-21b6b1a7c692', // unique identifier for the channel | ||
created: 1705012765708, // message creation time in ms | ||
id: 'b48e475e-1082-4c4d-a131-d90f8529390a' // unique identifier for this message | ||
}, | ||
payload: { // data specific to this event | ||
key: 'value' | ||
} | ||
} | ||
``` | ||
|
||
Secondly, we use a unique identifier for the sender and receiver. When a service widget | ||
cell is created, these identifiers are generated and transmitted to the widget app in | ||
the url. A `ReceiveChannel` will ignore any messages that do not contain channel id | ||
assigned to it and the channel id assigned to it's partner. | ||
|
||
#### Service Widget and Lifecycle | ||
|
||
##### Create new Service Widget Cell | ||
|
||
- Insert Narrative cell with the type `serviceWidget`, and the requisite cell | ||
initialization structure | ||
- The service widget cell manager will be listening for the notebook event | ||
`insertedAtIndex.Cell`, and will complete the cell's setup | ||
- This includes the usual runtime augmentation and monkeypatching, and python generation, | ||
insertion, and execution. | ||
- Cell management, or rather integration with the Narrative notebook workflow, is | ||
conducted by an instance of `CellManager`. This class can serve for any cell, but is | ||
only used for the serviceWidget at this time. | ||
- Cell integration with the Narrative notebook, KBase custom cell architecture, and | ||
overall is carried out by a subclass of `CellBase`. `CellBase` has most of the | ||
logic, the sublcass mostly fills in the blanks, such as stop and start behavior, and | ||
python code generation. | ||
- Most of the cell's application-specific behavior begins after the python code has been | ||
executed and has inserted the initial Javascript. | ||
- This initial Javascript invokes the `Root` widget in the service widget cell extension | ||
codebase, which in turn inserts the `Main` component, which prepares and inserts the | ||
`IFrame` component. | ||
- Once the iframe is inserted, it fetches the widget specified in the url. | ||
- This url includes the `iframeParams`, which are required for the iframe | ||
communication setup | ||
- It also includes the widget params | ||
- The auth token,if present, is sent passively as a cookie | ||
- The widget app loads in the iframe, and if all goes well sends a `ready` message to | ||
the cell. | ||
- The cell then sends a `start` message to the iframe, passing along, separately, the | ||
authentication, configuration, widget params, and widget state (not used in this | ||
implementation). This is provided primarily for purely static Javascript-based | ||
widgets. | ||
- The widget, upon processing the `start` message, sends a `started` message back to the | ||
cell. The only information passed in this message is the preferred height of the cell, | ||
which may be calculated after the widget has been rendered. | ||
|
||
After this, the widget proceeds to operate independently. | ||
|
||
> Note that state persistence, although fragments are in the codebase, is not yet | ||
> included in the the current implementation. | ||
|
||
#### Resizing with Persistence | ||
|
||
Since service widgets may be of arbitrary size we don't necessarily want the cell to | ||
expand to the full cell height. Additionally, a widget may be responsive - resizable - | ||
and have no natural, fixed height. Therefore, the widget cell is resizable, and will | ||
retain the set size when saved. Upon loading, the cell, if restore the height. | ||
|
||
This is part of the effort to have widget cell state preserved in the Narrative. | ||
|
||
### Data Viewer Support | ||
|
||
The dynamic service widget can serve as a data viewer widget. This is enabled by a | ||
specifically constructed NarrativeViewer app spec, and support added to | ||
`kbaseNarrativeWorkspace` that can recognize that app spec and construct the service | ||
widget cell. | ||
|
||
The viewer spec must provide an output widget name of `"ServiceWidget"`, | ||
and the output mapping must supply the service module name and widget name. This is | ||
detailed below. | ||
|
||
In addition, the object ref is added to the parameters without any need to have it | ||
specified in the viewer spec. It's presence is implied by the fact that it is in the | ||
context of a viewer. | ||
|
||
#### NarrativeViewer spec | ||
|
||
Narrative viewer widgets are defined in the `NarrativeViewers` quasi-service, and | ||
implemented in the Narrative as Javascript modules. The mapping of workspace type to | ||
widget is provided by the type spec. | ||
|
||
Each viewer is implemented as a quasi-method in the `NarrativeViewers` service. Viewers | ||
are not full methods as they have no implementation. The viewer methods only serve for | ||
their specifications. | ||
|
||
A viewer is specified in the `widgets.output` property of the `spec.json` method | ||
specification. The viewer corresponds to the AMD module name for the viewer in the | ||
Narrative codebase. | ||
|
||
Output parameters specified in `behavior.output_mapping` are processed in two ways - | ||
input parameters appearing in output are set with the ref or object name of the data | ||
object being rendered, others are processed supplied as parameters to the widget. | ||
|
||
For dynamic service widgets we utilize this framework in a specific manner: | ||
|
||
1. The `widget.output` property must have the value `"ServiceWidget"` | ||
2. The `behavior.output_mapping` must have two entries: | ||
1. One with the target property `service_module_name` and a `constant_value` which is | ||
the module name for the dynamic service serving the widget | ||
2. One with the target proerty `widget_name` and a `constant_value` which is the name | ||
of the widget with the dynamic service | ||
3. Any additional entries in `behavior.output_mapping` which specify a `constant_value` | ||
will be passed ot the widget, but none are required | ||
|
||
#### `kbaseNarrativeWorkspace.js` support | ||
|
||
Although `widget.output` is traditionally used as the actual module name of a viewer | ||
widget, for dynamic service widgets it serves to signal a specific type of dynamic | ||
service widget. The task of building a viewer cell resides in the | ||
`kbaseNarrativeWorkspace.js` module. | ||
|
||
### App Output Support | ||
|
||
Service widgets may also serve as viewers for app output. | ||
|
||
As for data viewers, enabling service widgets requires changes to the existing mechanism for handling app output. | ||
|
||
This functionality resides within the app cell itself - specifically the `createOutputCell` function in `nbextensions/appCell2/appCellWidget.js`. | ||
|
||
Similar to the data viewer cell, the implementation of the output viewer hinges on interceding before the cell type currently handling app output is inserted into the Narrative. | ||
|
||
And also similar to the data viewer cell, the app output specification requires a specific format to indicate that a service widget should be used, and provides the service widget's dynamic service module name and widget name. | ||
|
||
#### Summary of Changes and New Features | ||
|
||
App support required changes to the app cell widget, in the section in which it dispatches | ||
to the viewer widget, and icon support. | ||
|
||
Well, the latter may not stand the test of time, but I thought this could be a good | ||
opportunity to improve the app output icon. | ||
|
||
|
||
#### App Spec Format | ||
|
||
As for the data viewer usage of the service widget, the app output requires a specific | ||
format of app's ui specification. No new functionality is added, it is just that the | ||
service widget requires specific values be present. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
No need to rewrite, IMO.