-
Notifications
You must be signed in to change notification settings - Fork 21
Flyout: use theme default for position
#482
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
Conversation
src/flyout.js
Outdated
| this.position = | ||
| objectPath.get(this.config, "addons.flyout.position", null) || | ||
| this.position; |
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.
Seems clearer to set this in an if, instead of resetting the value to itself?
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.position = | |
| objectPath.get(this.config, "addons.flyout.position", null) || | |
| this.position; | |
| position = objectPath.get(this.config, "addons.flyout.position", null); | |
| if position: | |
| this.position = position; |
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.
My reading is "get the value from the API or keep the actual one".
I don't have a strong preference, but happy to change it if wanted.
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 don't feel super strongly, but think it's more explicit. Setting a value to itself is odd to me?
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.
position is a reactive property, so it's best to avoid triggering unneeded updates to the property. Wrapping this with a conditional is probably best. Otherwise this pattern for setting variable defaults is pretty common though.
|
@agjohnson I would appreciate your review on this PR so we can decide whether or not move forward with this implementation. |
src/flyout.js
Outdated
| this.position = | ||
| objectPath.get(this.config, "addons.flyout.position", null) || | ||
| this.position; |
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.
position is a reactive property, so it's best to avoid triggering unneeded updates to the property. Wrapping this with a conditional is probably best. Otherwise this pattern for setting variable defaults is pretty common though.
Initial POC to prove if this idea is possible and how we feel about it. I put together an example of what I understand we want to build here 😅 . In this example, the theme author will make explicitly how they want to use `readthedocs-flyout` HTML tag: ```html <readthedocs-flyout position="top-right"></readthedocs-flyout> ``` That will add the class `top-right` and show the flyout as follows:  However, the project author can change the `position` value from the dashboard if they prefer to override the theme's default value:  * Related #51 * Requires readthedocs/ext-theme#554 * Requires readthedocs/readthedocs.org#11891 * Example using https://test-builds.readthedocs.io/en/addons-theme-defaults/ * Closes #434
Initial POC to prove if this idea is possible and how we feel about it.
I put together an example of what I understand we want to build here 😅 . In this example, the theme author will make explicitly how they want to use
readthedocs-flyoutHTML tag:That will add the class
top-rightand show the flyout as follows:However, the project author can change the
positionvalue from the dashboard if they prefer to override the theme's default value: