Adding column type label to dropdowns #4566
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4566 +/- ##
==========================================
+ Coverage 71.2% 71.24% +0.04%
==========================================
Files 187 190 +3
Lines 14785 14875 +90
Branches 1083 1099 +16
==========================================
+ Hits 10527 10598 +71
- Misses 4255 4274 +19
Partials 3 3
Continue to review full report at Codecov.
|
| } else if (type.match(/.*time.*/i)) { | ||
| stringIcon = 'time'; | ||
| } else { | ||
| stringIcon = '?'; |
There was a problem hiding this comment.
I kinda feel like the null case should be blank rather than a ? to make things a bit cleaner
There was a problem hiding this comment.
Yeah I agree. I added the ? because there's a specific UNKNOWN type and was thinking maybe having ? as well as a blank could be confusing. But I agree they should either be separate or both blank.
| stringIcon = '?'; | ||
| } | ||
|
|
||
| const typeIcon = stringIcon === 'time' ? <i className="fa fa-clock-o text-muted type-label" /> : ( |
There was a problem hiding this comment.
nit: I think it would be a bit more readable to do
clause ?
option1 :
option2;
|
|
||
| export default function ColumnTypeLabel({ type }) { | ||
| let stringIcon; | ||
| let iconSize = '13'; |
There was a problem hiding this comment.
these should all be fine as #s not strings
| let stringIcon; | ||
| let iconSize = '13'; | ||
| if (type === '' || type === 'expression') { | ||
| stringIcon = 'ƒ'; |
| } | ||
|
|
||
| const typeIcon = stringIcon === 'time' ? <i className="fa fa-clock-o text-muted type-label" /> : ( | ||
| <div className="text-muted type-label" style={{ fontSize: iconSize }}>{stringIcon}</div>); |
There was a problem hiding this comment.
is the 2px ABC font size off enough that it warrants inline styles? ideally avoided since you're already using a new css class. I'd say a slightly larger font consistently across all icons (like 14px?) would be ideal aesthetically. I think they'd be differentiated enough from the dropdown text with the muted style you apply.
There was a problem hiding this comment.
To me, keeping the same default font for ABC seemed to compete a bit with the text next to it, but I don't have strong opinions.
There was a problem hiding this comment.
Updated the original comment with new screenshots based off our conversation.
michellethomas
force-pushed
the
add_type_icons_dropdown
branch
from
2b9b20f
to
2227d61
Compare
|
|
|
|
||
| let wrapper; | ||
| let props; | ||
| const factory = o => <ColumnTypeLabel {...o} />; |
There was a problem hiding this comment.
I know there are a lot of places that do this already, but single character variable names don't facilitate readability.
| const factory = o => <ColumnTypeLabel {...o} />; | ||
| beforeEach(() => { | ||
| wrapper = shallow(factory(defaultProps)); | ||
| props = Object.assign({}, defaultProps); |
There was a problem hiding this comment.
I think this pattern for test setup is difficult to read in terms of what props are actually applied and it seems strange to allow tests to modify the props object directly / reset them every test . I've also seen several code bases move away from using shared let variables because it can result in race conditions in more complex tests.
a more readable and common pattern I've seen is:
const props = { ...defaultProps };
describe('MyComponent', () => {
function setup(overrides) {
const wrapper = shallow(<MyComponent {...props} {...overrides} />);
return wrapper;
}
it('does something', () => {
const wrapper = setup(/* { optional: override } */);
expect(...);
});
...
});
|
LGTM |
* Adding column type label to dropdowns * Changing the style of column type label * Adding tests for ColumnTypeLabel * Adding tests for time and fixing if statement order * Changing location of ColumnTypeLabel tests * Updating ColumnTypeLabel structure
* Adding column type label to dropdowns * Changing the style of column type label * Adding tests for ColumnTypeLabel * Adding tests for time and fixing if statement order * Changing location of ColumnTypeLabel tests * Updating ColumnTypeLabel structure
mistercrunch
added
🏷️ bot
Adding column type information to the optionRenderer for any controls that use ColumnOption or MetricOption. So this will show up in the dropdown but not inline in the value renderer. This deviates from our original designs but it was making the page look pretty busy when I added it to our current page.
Would love to get more design feedback!
In the future, in the metrics section we can distinguish between aggregations and columns. Also, once some of the metrics work Gabe is doing gets in, I'd like to add this to all dropdowns for consistency.
@GabeLoins @williaster @mistercrunch