-
Notifications
You must be signed in to change notification settings - Fork 941
Refactor About dialog to remove HTML template and use centralized server info #13933
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
base: main
Are you sure you want to change the base?
Conversation
|
Todo
|
af2619c to
d26bde0
Compare
d26bde0 to
8c74743
Compare
This adds centralized storage for: - COOLWSD version and git hash (from 'coolserver') - LOKit (LibreOffice) version and git hash (from 'lokitversion') - OS info (from 'osinfo') This provides a single source of truth for server identification data, making it globally available for the About dialog and future UI components, without relying on DOM cloning or hidden HTML template. - Add data-wopi-host-id, data-vendor, and data-copyright-year attributes to #initial-variables Signed-off-by: Banobe Pascal <[email protected]> Change-Id: I7308db0b3fec7ea335d62108614fe4574af0d1c2
8c74743 to
6b2fdf0
Compare
|
|
||
| // This method can be used in the future to populate the about dialog content | ||
| private static populateAboutDialog(content: HTMLElement): void { | ||
| const windowAny = window as any; |
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.
there is an extended Window type: https://github.com/CollaboraOnline/online/blob/main/browser/src/global.d.ts#L188
we can add missing fields there and use it instead of any to have more hardened code
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, I have added the changes
|
|
||
| private copyVersionInfoToClipboard() { | ||
| const info = window.app.serverInfo; | ||
| const windowAny = window as any; |
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.
do not use any
| ); | ||
|
|
||
| // COOLWSD version | ||
| const coolwsdEl = content.querySelector('#coolwsd-version') as HTMLElement; |
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.
if we query element by id coolwsd-version while we create it below in
https://github.com/CollaboraOnline/online/pull/13933/files#diff-4a2c10a9a863dfc9898b33b5a26feb9ad2038506f432792ccb8ffcd1d9357080R521
We could use variable for the ID (like AboutDialog.VersionFieldId etc.) so we are sure relation will not be broken.
But even better would be: do not query but save reference to the element we created in the variable and use in the other function :)
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.
we could avoid all these checks: if (element) { ...} as we will be sure they exist
| const content: HTMLElement = AboutDialog.createAboutDialogContent(); | ||
| content.style.display = 'block'; | ||
|
|
||
| AboutDialog.populateAboutDialog(content); |
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.
In above AboutDialog.createAboutDialogContent we create fields, which later are used in AboutDialog.populateAboutDialog.
We could return: const content: { versionLabel: HTMLElement, hashLabel: HTMLElement, etc... } = AboutDialog.createAboutDialogContent();
and then AboutDialog.populateAboutDialog(content); could use them without query :)
| // This method can be used in the future to populate the about dialog content | ||
| private static populateAboutDialog(content: HTMLElement): void { | ||
| const windowAny = window as any; | ||
| const info = window.app.serverInfo; |
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 we do a validation below and log ( app.console.error('....') ) + early return in case of missing info ?
that will save us all these ifs later...
- Dynamically create the UI structure in JavaScript - Populate all content from window.app.serverInfo - Eliminate cloning from hidden HTML template - Separate structure creation from content population and show - Improve copy-to-clipboard to use serverInfo directly and include all fields (versions, hashes, served-by, server ID) - Use window.wopiHostId, window.copyrightYear, and window.vendor for accurate WOPI host, current year, and vendor name display - Preserve all other existing functionality Signed-off-by: Banobe Pascal <[email protected]> Change-Id: I1efadd2d106150dd8b1c973c82d3d2c12ae0762d
The About dialog is now fully created and filled in JavaScript using: - createAboutDialogContent() to build the structure - window.app.serverInfo to provide version and server details - populateAboutDialog() to insert all content and branding - This removes the need for duplicated HTML in the template file and makes the dialog easier to maintain. - A minimal empty <div id=\"about-dialog\" tabIndex=\"0\"></div> is kept in the HTML to preserve Notebookbar About tab detection Signed-off-by: Banobe Pascal <[email protected]> Change-Id: Id0bea4b6892b62fe4a9396c8d941f79ee5e39b97
6b2fdf0 to
13720c4
Compare
Summary
TODO
Checklist
make prettier-writeand formatted the code.make checkmake runand manually verified that everything looks okay