rusackas commented on a change in pull request #11528:
URL:
https://github.com/apache/incubator-superset/pull/11528#discussion_r516199483
##########
File path: superset-frontend/spec/javascripts/components/Menu_spec.jsx
##########
@@ -18,7 +18,8 @@
*/
import React from 'react';
import { shallow, mount } from 'enzyme';
-import { Nav, MenuItem } from 'react-bootstrap';
+import { Nav } from 'react-bootstrap';
+import { Menu as DropdownMenu } from 'src/common/components';
Review comment:
VERY minot question (non-blocking), but I'm wondering if just letting it
be Menu makes sense... when we could find all the `Menu.Item` within the
codebase if we ever needed to.
##########
File path: superset-frontend/src/common/components/index.tsx
##########
@@ -43,6 +43,11 @@ export const MenuItem = styled(AntdMenu.Item)`
> a {
text-decoration: none;
}
+
+ &.ant-menu-item {
+ height: 30px;
+ line-height: 30px;
+ }
Review comment:
Are there any Menu.Items that do _not_ have the `.ant-menu-item` class?
If it's all the same, it might be cleaner to do this block like so:
```suggestion
height: 30px;
line-height: 30px;
> a {
text-decoration: none;
}
```
Also, it might make sense for the height/line-height to be based on
typography, i.e. if the font-size is `theme.typography.sizes.m` (14px) then
maybe these `30px` values should be `theme.typography.sizes.m * 2`
##########
File path: superset-frontend/spec/javascripts/components/Menu_spec.jsx
##########
@@ -18,7 +18,8 @@
*/
import React from 'react';
import { shallow, mount } from 'enzyme';
-import { Nav, MenuItem } from 'react-bootstrap';
+import { Nav } from 'react-bootstrap';
+import { Menu as DropdownMenu } from 'src/common/components';
Review comment:
VERY minor question (non-blocking), but I'm wondering if just letting it
be Menu makes sense... when we could find all the `Menu.Item` within the
codebase if we ever needed to.
##########
File path: superset-frontend/src/common/components/index.tsx
##########
@@ -43,6 +43,11 @@ export const MenuItem = styled(AntdMenu.Item)`
> a {
text-decoration: none;
}
+
+ &.ant-menu-item {
+ height: 30px;
+ line-height: 30px;
+ }
Review comment:
This same issue appeared on another PR:
https://github.com/apache/incubator-superset/pull/11487
One way or another, we should fix it there too :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
