-
Notifications
You must be signed in to change notification settings - Fork 120
DOCSP-877 & DOCSP-879 - Parameterize filter and update saved pref. #161
Conversation
sphinxext/tabs.py
Outdated
<ul class="tab-strip tab-strip--singleton" role="tablist"> | ||
{{ for tab in tabs sortLanguages }} | ||
<ul class="tab-strip tab-strip--singleton" role="tablist" PREFERENCE> | ||
{{ for tab in tabs FILTER }} |
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 we use some kind of notation with special characters or something to make it less like plain english? For example, $preference
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 would go with e.g. %PREFERENCE%
, yeah
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.
Done
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.
A couple of suggestions, but largely looks fine!
* @param {object} value The "tabId" and optional "type" (tab set) | ||
*/ | ||
set tabPref(value) { | ||
const tabPref = this.tabPref; |
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'd not make this local variable
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.
Discussed offline, calling the getter method above.
* @returns {object} Tab preference object. | ||
*/ | ||
get tabPref() { | ||
let pref = JSON.parse(window.localStorage.getItem(this.key)); |
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.
Consider making this just
return JSON.parse(window.localStorage.getItem(this.key)) || {};
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.
Done
b80da59
to
33a1b10
Compare
MUST BE MERGED WITH:
mongodb/docs#3074