[GitHub] williaster commented on a change in pull request #4566: Adding column type label to dropdowns
williaster commented on a change in pull request #4566: Adding column type
label to dropdowns
URL:
https://github.com/apache/incubator-superset/pull/4566#discussion_r173588482
##
File path: superset/assets/spec/javascripts/components/ColumnTypeLabel_spec.jsx
##
@@ -0,0 +1,62 @@
+import React from 'react';
+import { expect } from 'chai';
+import { describe, it } from 'mocha';
+import { shallow } from 'enzyme';
+
+import ColumnTypeLabel from '../../../javascripts/components/ColumnTypeLabel';
+
+describe('ColumnOption', () => {
+ const defaultProps = {
+type: 'string',
+ };
+
+ let wrapper;
+ let props;
+ const factory = o => ;
Review comment:
I know there are a lot of places that do this already, but single character
variable names don't facilitate readability.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] williaster commented on a change in pull request #4566: Adding column type label to dropdowns
williaster commented on a change in pull request #4566: Adding column type
label to dropdowns
URL:
https://github.com/apache/incubator-superset/pull/4566#discussion_r173589480
##
File path: superset/assets/spec/javascripts/components/ColumnTypeLabel_spec.jsx
##
@@ -0,0 +1,62 @@
+import React from 'react';
+import { expect } from 'chai';
+import { describe, it } from 'mocha';
+import { shallow } from 'enzyme';
+
+import ColumnTypeLabel from '../../../javascripts/components/ColumnTypeLabel';
+
+describe('ColumnOption', () => {
+ const defaultProps = {
+type: 'string',
+ };
+
+ let wrapper;
+ let props;
+ const factory = o => ;
+ beforeEach(() => {
+wrapper = shallow(factory(defaultProps));
+props = Object.assign({}, defaultProps);
Review 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:
```javascript
const props = { ...defaultProps };
describe('MyComponent', () => {
function setup(overrides) {
const wrapper = shallow();
return wrapper;
}
it('does something', () => {
const wrapper = setup(/* { optional: override } */);
expect(...);
});
...
});
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] williaster commented on a change in pull request #4566: Adding column type label to dropdowns
williaster commented on a change in pull request #4566: Adding column type
label to dropdowns
URL:
https://github.com/apache/incubator-superset/pull/4566#discussion_r173007182
##
File path: superset/assets/javascripts/components/ColumnTypeLabel.jsx
##
@@ -0,0 +1,34 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+
+const propTypes = {
+ type: PropTypes.string.isRequired,
+};
+
+export default function ColumnTypeLabel({ type }) {
+ let stringIcon;
+ let iconSize = '13';
+ if (type === '' || type === 'expression') {
+stringIcon = '?';
+ } else if (type.match(/.*char.*/i) || type.match(/string.*/i) ||
type.match(/.*text.*/i)) {
+stringIcon = 'ABC';
+iconSize = '11';
+ } else if (type.match(/.*int.*/i) || type === 'LONG' || type === 'DOUBLE') {
+stringIcon = '#';
+ } else if (type.match(/.*bool.*/i)) {
+stringIcon = 'T/F';
+ } else if (type.match(/.*time.*/i)) {
+stringIcon = 'time';
+ } else {
+stringIcon = '?';
+ }
+
+ const typeIcon = stringIcon === 'time' ? : (
+{stringIcon});
Review 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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] williaster commented on a change in pull request #4566: Adding column type label to dropdowns
williaster commented on a change in pull request #4566: Adding column type
label to dropdowns
URL:
https://github.com/apache/incubator-superset/pull/4566#discussion_r173005337
##
File path: superset/assets/javascripts/components/ColumnTypeLabel.jsx
##
@@ -0,0 +1,34 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+
+const propTypes = {
+ type: PropTypes.string.isRequired,
+};
+
+export default function ColumnTypeLabel({ type }) {
+ let stringIcon;
+ let iconSize = '13';
+ if (type === '' || type === 'expression') {
+stringIcon = '?';
Review comment:
love this!
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] williaster commented on a change in pull request #4566: Adding column type label to dropdowns
williaster commented on a change in pull request #4566: Adding column type
label to dropdowns
URL:
https://github.com/apache/incubator-superset/pull/4566#discussion_r173005251
##
File path: superset/assets/javascripts/components/ColumnTypeLabel.jsx
##
@@ -0,0 +1,34 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+
+const propTypes = {
+ type: PropTypes.string.isRequired,
+};
+
+export default function ColumnTypeLabel({ type }) {
+ let stringIcon;
+ let iconSize = '13';
Review comment:
these should all be fine as #s not strings
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
