You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/09/30 01:50:27 UTC
[GitHub] [druid] vogievetsky opened a new pull request #10454: Web console: switch to switches instead of checkboxes
vogievetsky opened a new pull request #10454:
URL: https://github.com/apache/druid/pull/10454
There was a styling regression in the web console that made the tick in the checkbox box not render.
This is what it looked like in previously:
After looking into it I decided to switch away from using blueprint's `Checkbox` control.
For lists (like the one above) a more blueprint idiomatic way is to set an icon on a `MenuItem` this way does not require any css hacks.
This is how it looks:
For all other places where a Checkbox was used (log tailing in the ShowLogs dialog and WarningChecklist) I switched the control to a switch - which I think is nicer because it is bigger and more satisfying to use. It is also more common throughout the UI.
I think it is an improvement in all cases.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10454: Web console: switch to switches instead of checkboxes
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10454:
URL: https://github.com/apache/druid/pull/10454#discussion_r497738219
##########
File path: web-console/src/components/menu-checkbox/menu-checkbox.tsx
##########
@@ -16,15 +16,26 @@
* limitations under the License.
*/
-import { Checkbox, ICheckboxProps } from '@blueprintjs/core';
+import { MenuItem } from '@blueprintjs/core';
+import { IconNames } from '@blueprintjs/icons';
import React from 'react';
-import './menu-checkbox.scss';
+export interface MenuCheckboxProps {
+ text: string;
+ checked: boolean;
+ onChange: () => void;
+}
+
+export function MenuCheckbox(props: MenuCheckboxProps) {
+ const { text, checked, onChange } = props;
-export function MenuCheckbox(props: ICheckboxProps) {
return (
- <li className="menu-checkbox">
- <Checkbox {...props} />
- </li>
+ <MenuItem
+ className="menu-checkbox"
+ icon={checked ? IconNames.TICK : IconNames.BLANK}
Review comment:
I agree. This looks much better
Thanks for suggestion!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky merged pull request #10454: Web console: switch to switches instead of checkboxes
Posted by GitBox <gi...@apache.org>.
vogievetsky merged pull request #10454:
URL: https://github.com/apache/druid/pull/10454
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #10454: Web console: switch to switches instead of checkboxes
Posted by GitBox <gi...@apache.org>.
ccaominh commented on a change in pull request #10454:
URL: https://github.com/apache/druid/pull/10454#discussion_r497694299
##########
File path: web-console/src/components/menu-checkbox/menu-checkbox.tsx
##########
@@ -16,15 +16,26 @@
* limitations under the License.
*/
-import { Checkbox, ICheckboxProps } from '@blueprintjs/core';
+import { MenuItem } from '@blueprintjs/core';
+import { IconNames } from '@blueprintjs/icons';
import React from 'react';
-import './menu-checkbox.scss';
+export interface MenuCheckboxProps {
+ text: string;
+ checked: boolean;
+ onChange: () => void;
+}
+
+export function MenuCheckbox(props: MenuCheckboxProps) {
+ const { text, checked, onChange } = props;
-export function MenuCheckbox(props: ICheckboxProps) {
return (
- <li className="menu-checkbox">
- <Checkbox {...props} />
- </li>
+ <MenuItem
+ className="menu-checkbox"
+ icon={checked ? IconNames.TICK : IconNames.BLANK}
Review comment:
If none of the menu items are checked, there's no visual clue that the menu item has a checkbox since it's blank. Maybe something like the `TICK CIRCLE` / `CIRCLE` icons could be used instead? Or, if this was changed to use the `Switch` component (like the other changes you made in the PR), would it have similar styling issues?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org