Improve padding logic for updatemenus #989
Conversation
|
Note that I did not set a minimum value: 0 for padding. Since we don't share the full generality of the html box model, I feel like negative padding could be a usefully obscure option to have available since we don't have pixel margins to worth with. |
|
Relayout test to be added; battery is at 5% so will be up, but not right at the moment. Otherwise complete and reviewable. |
| @@ -11,6 +11,7 @@ | |||
| var fontAttrs = require('../../plots/font_attributes'); | |||
| var colorAttrs = require('../color/attributes'); | |||
| var extendFlat = require('../../lib/extend').extendFlat; | |||
| var padAttrs = require('../../plots/pad_attributes'); | |||
There was a problem hiding this comment.
That's where pad_attributes.js belongs at the moment, but I hate to see so many files pile up in the src/plots/ root.
Maybe we could add a src/plots/common/folder? or src/components/common/? or even a src/common/ for similar shared modules.
There was a problem hiding this comment.
Agreed. Font attributes would probably go with it? At least internal reorg is easy refactoring (especially since static requires so that errors fail bundling), so can try to cut down on plots/ bit by bit (pun?)
There was a problem hiding this comment.
Font attributes would probably go with it?
yep, exactly.
so can try to cut down on plots/ bit by bit (pun?)
No action required in this PR. I just wanted to mention something I've been thinking about for a while now.
| l: paddedWidth * ({right: 1, center: 0.5}[xanchor] || 0), | ||
| r: paddedWidth * ({left: 1, center: 0.5}[xanchor] || 0), | ||
| b: paddedHeight * ({top: 1, middle: 0.5}[yanchor] || 0), | ||
| t: paddedHeight * ({bottom: 1, middle: 0.5}[yanchor] || 0) |
There was a problem hiding this comment.
Wow. That was easy. I love it!
|
@etpinard will do. Have run out of steam at the moment, but will aim to have it good to go first thing tomorrow. |
|
ping @etpinard — added relayout tests that at least confirms a couple basic things about padding getting updated and making some sense. |
|
This is working great. Thanks! 💃 @rreusser merge away |
See #985
cc @etpinard
This PR improves padding logic for updatemenus. It implements
pad: {t, r, b, l}at the individual-updatemenu level. I didn't apply unit testing, but did beef up the image mock to really ensure that I've tested the permutations.The general logic:
The order of operations makes a slight difference, but the general concept is that this applies within the container first, then to the overall layout as expected. I think this is by far the most intuitive and easy way to accomplish this, and meets the need.
Unfortunately it's not that straightforward to document image mocks, and I'd rather not have (literally) sixty mocks to verify all aspects of the positioning, and I think it's best to avoid testing visual alignment in the unit tests.
So on that note, here's the improved mock with the anchor annotated in blue and padding annotated in red: