Improve xAxis ticks, thinner bottom margin #4756
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4756 +/- ##
=========================================
- Coverage 72.64% 72.4% -0.25%
=========================================
Files 205 207 +2
Lines 15402 15494 +92
Branches 1183 1194 +11
=========================================
+ Hits 11189 11218 +29
- Misses 4210 4273 +63
Partials 3 3
Continue to review full report at Codecov.
|
| } else if (fd.x_ticks_layout === 'staggered') { | ||
| staggerLabels = true; | ||
| } else if (fd.x_ticks_layout === '45°') { | ||
| if (fd.show_brush === true || fd.show_brush === 'yes') { |
There was a problem hiding this comment.
This seems to be a common pattern when we add more options to a control (usually when "auto" is added to an on/off control). Maybe we should have a function called isTruthy, eg:
function isTruthy(obj) {
if (typeof variable === 'boolean') {
return obj;
} else if (typeof variable === 'string') {
return obj.toLowerCase() !== 'no';
} else {
return !!obj;
}
}Or maybe not. Just thinking out loud. :)
| } | ||
| const showBrush = ( | ||
| fd.show_brush === true || | ||
| fd.show_brush === 'yes' || |
There was a problem hiding this comment.
^ here it is, the other use for isTruthy
|
I agree on the [un] manageability of this file. The challenge is that the different visualizations both from a controls and API standpoint have a lot in common. A lot of code is shared in odd intricate ways. It's a though puzzle. A balanced way to evolve this would be to refactor out portion of the code into smaller functions an unit test those. We've been talking about moving to |
|
@graceguo-supercat @michellethomas would love for someone at Airbnb to sign off on this as it will affects a million line charts over there, but that should be for the best. |
mistercrunch
force-pushed
the
bottom_margin
branch
from
c115a8c to
14102d3
Compare
|
great addition. my only suggestion at this point is setting as discussed offline, in the future it'd be useful to add a "concise"/"compact" version of the auto time format to help fix this problem. this could be used for axes and the "detailed" version could be used in tooltips. |
|
I'd also prefer Maybe next PR on this we'd go:
The magic formatting is the part where the devil is in the detail and people may not agree on the exact layout. Here's a link to the code we have now: Oh and we haven't even talked about locales :( |
|
is there a rush to merge this? would it be worthwhile to add the shorter date formatting in this PR and set the default to the main concern I have is with saved slices, by merging this |
|
Actually the default setting is |
|
But yeah, while this doesn't fix everything, it addresses many visualizations defects and bugs with the xAxis and makes much better use of real estate. It also does not change the content of the labels which could be controversial and could be considered breaking backwards compatibility. It also improves the organization of axis-related controls for many chart types, making more consistent. |
|
ah, that's nice about the |
|
@williaster I agree with You have my blessing to figure out better defaults that will please a majority of users. One key point is that day-of-week is very important to both Airbnb and Lyft given the strong weekly cycles, I wouldn't kick this out, better removing |
|
I'll wait on @graceguo-supercat's review before merging this |
| @@ -86,3 +86,12 @@ export function getShortUrl(longUrl, callback) { | |||
| }, | |||
| }); | |||
| } | |||
|
|
|||
| export function isTruthy(obj) { | |||
There was a problem hiding this comment.
acknowledging I have very little context here- this seems like a bad road to go down. We use YAML for plenty of stuff over here and there have been many issues caused by someone not expecting True/t/yes to be true...
There was a problem hiding this comment.
It has to do with backward compatibility here. There's been a few instances of CheckboxControl becoming a SelectControl. In this case the show_brush used to be checkbox and now is auto, Yes or 'No'.
Since it's not the first time and won't be the last, we decided to go this route.
The alternatives would be to either:
- write database migrations to alter all slices and replace
Truewith'Yes'but that's more work and db migration have maintenance overhead. - systematically use
if (fd.show_brush === true || fd.show_brush === 'yes')everywhere
mistercrunch
force-pushed
the
bottom_margin
branch
from
14102d3 to
e6ea1f7
Compare
|
@mistercrunch This PR works good in airbnb data! please merge whenever you feel comfortable. Thanks! |
* Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy
* Improve xAxis ticks, thinner bottom margin (#4756) * Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy * Addressing comments * Set cell_padding to 0 * merging db migrations
* Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy
* Improve xAxis ticks, thinner bottom margin (apache#4756) * Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy * Addressing comments * Set cell_padding to 0 * merging db migrations
* Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy
* Improve xAxis ticks, thinner bottom margin (apache#4756) * Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy * Addressing comments * Set cell_padding to 0 * merging db migrations
* Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy
* Improve xAxis ticks, thinner bottom margin (apache#4756) * Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy * Addressing comments * Set cell_padding to 0 * merging db migrations
mistercrunch
added
🏷️ bot
Introducing a new control
X Tick Layoutwith options['auto', 'flat', '45°', 'staggered']for nvd3 visualizations.nvd3 visualizations now do a much better job at minimizing the bottom margin in
automode, and give more control to the user.Also refactored quite a bit and aligned the controls across charts.
closes #4751