-
Notifications
You must be signed in to change notification settings - Fork 972
Add masterpagecurrent_icons in the noteboookbar #14391
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
|
please rebase on top of |
|
|
||
| JSDialog.notebookbarIconViewList = function ( | ||
| parentContainer: Element, | ||
| data: 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.
please add dedicated type for it in Definitions.Types.ts, it helps as a documentation and prevents mistakes
| commonContainer, | ||
| ); | ||
| iconviewsWrapper.id = 'masterpage_icons-wrapper'; | ||
| for( let i = 0; i < data.items.length; i++) |
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 that loop the only difference we need?
I would suggest to not copy paste code. It's duplicated and change in one instance requires change in other too. Best would be to share a code...
How?
- please use Object Oriented Programming here, and some trick:
- wrap all original functions in a class
NotebookbarIconViewList(parent class is more general) - extract the inner icon view creation (this commented piece) in a separate function which takes array of icon views data
- please create subclass which extends the original one as
NotebookbarIconView(single, narrower functionality), change the constructor so it takes regular icon view data and reuse parent class, just convert JSON to your "iconviewlist" format :)
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.
removed duplicate code by wrapping notebookbarIconViewList with notebookbarIconView. Let me know if you'd prefer OOP
| { | ||
| 'id': 'masterpageall_icons', // has to match core id | ||
| 'type': 'iconview', | ||
| 'label': 'Presentation Templates' |
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.
label should be translatable: _( ...text... )
|
Previews are not shown on first opening of a dropdown: This is caused by eventing design in JSDialog... we need to find inside a component the widget with On demand rendering requests previews when item is shown. Then when we receive response we need to find the widget. But your new widget is not the thing we will find. We try to apply change in "child iconviews" directly (id= but I think the dropdown container id is not good in new case? |
e157312 to
8fa47f0
Compare
fixed |
8fa47f0 to
3bc45f6
Compare
Signed-off-by: Shardul Vikram Singh <[email protected]> Change-Id: Id30888c170500bd6dfc369fac6b1efe5da17296d
3bc45f6 to
537f76c
Compare
Change-Id: Id30888c170500bd6dfc369fac6b1efe5da17296d
Summary
TODO
Checklist
make prettier-writeand formatted the code.make checkmake runand manually verified that everything looks okay