-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: toggle for wiki opening info display #15662
feat: toggle for wiki opening info display #15662
Conversation
ui/analyse/src/wiki.ts
Outdated
const plyPrefix = (node: Tree.Node) => | ||
`${Math.floor((node.ply + 1) / 2)}${node.ply % 2 === 1 ? '._' : '...'}`; | ||
|
||
const wikiBooksUrl = 'https://en.wikibooks.org'; | ||
const apiArgs = 'redirects&origin=*&action=query&prop=extracts&formatversion=2&format=json&exchars=1200'; | ||
const apiArgs = 'redirects&origin=*&action=query&prop=extracts&formatversion=2&format=json&exchars=1000'; |
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 with the implementation of the fieldset, some width was decreased for the text content, I kept the character length 1000 not to have a scrollbar for most cases, though after implementation I also felt, the scrollbar could rather be kept within the fieldset component and be much better. Please let me know your thoughts to confirm the suitability.
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 the scroll should be inside the fieldset. And the margin around the fieldset removed.
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.
Updated, thanks.
ui/analyse/src/wiki.ts
Outdated
$(window).on('load', () => { | ||
const wikiBookField = $('#wikibook-field').first(); | ||
const wikiBookStorageKey = 'analyse.wikibooks.display'; | ||
const toggleWikiBookStorage = () => site.storage.boolean(wikiBookStorageKey).toggle(); |
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.
Actually, I wanted to do this in the best possible way using Scala features like hook, or typescript snabbdom library, but couldn't come across a better solution. Some assistance here provided kindly might be really helpful. Regards.
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.
This file uses plain javascript DOM manipulation, so it makes sense to continue in that direction, as you did.
You don't need $(window).on('load'
.
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.
Actually, I tried it without the load
event earlier as well and again, but couldn't understand, the event doesn't trigger without it.
Even though the element already exists, any idea, why it could be so, else I may invest some more time to analyze this. Regards.
@@ -1,4 +1,5 @@ | |||
@import '../analyse.abstract'; | |||
@import '../../../common/css/component/mselect'; | |||
@import '../../../common/css/form/form3'; |
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 analysis board doesn't need form components, so I think the fieldset styling should be extracted to a new scss file, which form3 imports.
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.
Extracted to a reusable scss file: ui/common/css/component/_toggle-box.scss
Updated.
ui/analyse/src/wiki.ts
Outdated
site.pubsub.emit('chat.resize'); | ||
}; | ||
|
||
$(window).on('load', () => { | ||
const wikiBookField = $('#wikibook-field').first(); | ||
const wikiBookStorageKey = 'analyse.wikibooks.display'; |
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.
const wikiBookStorage = site.storage.boolean('analyse.wikibooks.display')
which you can then use like wikiBookStorage.toggle()
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.
Updated, thanks.
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.
looking pretty good!
- ui improvements for toggle box container - separate implementation for css styles and logic
7eff495
to
f4642e3
Compare
Closes #10769
This PR implements:
Local Test Link: http://localhost:9663/analysis
Other implementation discussions in the comments