-
Notifications
You must be signed in to change notification settings - Fork 830
Forms: Fix slider value position in editor #45218
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
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
const percent = ( ( value - min ) * 100 ) / ( max - min ); | ||
// Magic numbers: 16px base offset, 0.32px per percent | ||
return `calc(${ percent }% + (${ 16 - percent * 0.32 }px))`; | ||
} |
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 there's a better place for shared JS utility methods to live, or if we would rather create a catchall util.js file and put methods in that, let me know.
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.
Since the util is specific to the slider field (with magic numbers and all), might make sense to keep it in the slider field folder and share between front/back from there?
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, we could do that. In terms of code organization, the code is used in in both blocks and module folders here:
forms/src/blocks/input-range/edit.js
forms/src/modules/field-slider/view.js
One thing I've been meaning to ask, is why we put view.js in a separate modules folder? I think traditionally in block development, a block's view.js (it's frontend JS) just lives in the block folder along with other block related code.
Any opposition to moving the view.js to the block folder (may have to update some config somewhere too)? Then frontend JS, editor JS, and any utils would all be consolidated.
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's a separate discussion on slack now about view.js loading from block folder. For this PR, to keep things moving, I've just moved the shared method to a util.js folder in the input-range block folder. So it lives close to the block code, and then we just import it to view.js from there. Hopefully we'll eventually move view.js to the block folder too.
Since I already had approval on this PR and just move the method definition, once tests pass, I'll go ahead and merge.
Code Coverage SummaryCoverage changed in 2 files.
1 file is newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
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 changed worked well for me!
ff5dae3
to
3b1e66e
Compare
Proposed changes:
While testing another PR, I found the value bubble over the slider was mispositioned in the editor, but not the frontend. I think this occurred when we consolidated CSS for editor/frontend into a single file. We have two separate method for calculating slider position in editor/frontend, and I think one was updated and the other was not.
This PR creates a single utility method for calculating slider position, and imports and uses it in both editor and frontend. That fixes the original issue, but also the code duplication.
Before: Note value bubble is too far left at 0 and too far right at 100


After: Note value bubble is always centered over slider bullet


Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: