Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve button link usage #5409

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e1512c1
Refactor of link component
rafawendel Feb 5, 2021
9a7810a
Replaced uses of anchor to Link component
rafawendel Feb 8, 2021
ab7a9d2
Removed poor use of ButtonLink
rafawendel Feb 12, 2021
533995e
Added custom Button component
rafawendel Feb 10, 2021
83dc7ab
Replaced Link role button instances with Button type plain
rafawendel Feb 10, 2021
c18da8f
Applied anchor-is-valid to Link component
rafawendel Feb 10, 2021
d6ac6f1
Improved uses and removed redundancies from buttons
rafawendel Feb 10, 2021
3c27c32
Updated opportunistic uses for Link and Button
rafawendel Feb 10, 2021
b3361e5
Split PlainButton and ant Button
rafawendel Feb 12, 2021
44c7184
Fixed types for CardsList
rafawendel Feb 12, 2021
8a98959
Trigger build
rafawendel Mar 1, 2021
85f2831
Merge branch 'master' of github.com:getredash/redash into improve-but…
rafawendel Mar 4, 2021
93ff830
Removed buttons inside menuitems
rafawendel Mar 4, 2021
90f6c88
Added link like plain button
rafawendel Mar 4, 2021
fdd5bb1
Changed to imported styles
rafawendel Mar 5, 2021
137a9ba
Reverted improper ul usage
rafawendel Mar 5, 2021
532cdb2
Inverted mistakenly swapped ternary
rafawendel Mar 5, 2021
f2dfd06
Restored mistakenly removed Button
rafawendel Mar 5, 2021
e710faf
Added link style button to proper elements
rafawendel Mar 8, 2021
3e24e0c
Revert "Removed buttons inside menuitems"
rafawendel Mar 8, 2021
0caf343
Added styles for plain button inside menu item
rafawendel Mar 8, 2021
b281eff
Added styles for focus in menuitems
rafawendel Mar 8, 2021
c07546e
Added style to favorite star
rafawendel Mar 8, 2021
907f37c
Removed redundant style
rafawendel Mar 8, 2021
8afaa37
Improved appearance and accessibility of widgets
rafawendel Mar 8, 2021
31ee6b0
Removed accidentally added classes
rafawendel Mar 8, 2021
8c49157
Fixed accessibility and behavior of edit in place
rafawendel Mar 8, 2021
169761c
Fixed unwanted visual changes, added a11y friendly styles, improved m…
rafawendel Mar 8, 2021
646ed3e
Replaced imporper uses of <Button type="linK">
rafawendel Mar 8, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions client/app/assets/less/ant.less
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,18 @@
left: 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving a comment here so we can keep a discussion in the thread.

Is there any chance we can break this PR into even smaller PRs? It looks like there are quite a few styling changes, as well as component updates. I'm having trouble determining how best to QA / ensure there are no regressions.

Do you have another proposal? I don't want to stonewall you or anything, but I don want to make sure I can limit the number of iterations / time spent verifying changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's complicated. I did think about it, but the thing is that this PR resulted in a lot of visual changes, which I had to solve in order to be mergeable. What is sort of possible but somewhat weird is having the fixes for those in master before we merge it. LMK what is your opinion on that.

An alternative is to review those last commits primarily, since they are where the styling changes are amassed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just did one button type at a time?

}
}

&:focus,
&:focus-within {
color: @menu-highlight-color;
}
}
}

.@{dropdown-prefix-cls}-menu-item {
&:focus,
&:focus-within {
background-color: @item-hover-bg;
}
}

Expand Down
4 changes: 4 additions & 0 deletions client/app/assets/less/inc/base.less
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ strong {
cursor: pointer;
}

.clickable:disabled {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure there's an action to take, yet, but I have mixed feelings about this. Anything can take the classname clickable, but not anything can be disabled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about .clickable button:disabled?

cursor: not-allowed;
}

.resize-vertical {
resize: vertical !important;
transition: height 0s !important;
Expand Down
31 changes: 14 additions & 17 deletions client/app/assets/less/inc/edit-in-place.less
Original file line number Diff line number Diff line change
@@ -1,26 +1,23 @@
.edit-in-place span {
.edit-in-place {
white-space: pre-line;
display: inline-block;

p {
margin-bottom: 0;
}
}

.edit-in-place span.editable {
display: inline-block;
cursor: pointer;
}

.edit-in-place span.editable:hover {
background: @redash-yellow;
border-radius: @redash-radius;
}
.editable {
display: inline-block;
cursor: pointer;

.edit-in-place.active input,
.edit-in-place.active textarea {
display: inline-block;
}
&:hover {
background: @redash-yellow;
border-radius: @redash-radius;
}
}

.edit-in-place {
display: inline-block;
&.active input,
&.active textarea {
display: inline-block;
}
}
11 changes: 8 additions & 3 deletions client/app/assets/less/inc/table.less
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,19 @@
color: #d4d4d4;
transition: all 0.25s ease-in-out;

.fa-star {
color: @yellow-darker;
}

&:hover,
&:focus {
color: @yellow-darker;
cursor: pointer;
}

.fa-star {
color: @yellow-darker;
.fa-star {
filter: saturate(75%);
opacity: 0.75;
}
}
}

Expand Down
6 changes: 4 additions & 2 deletions client/app/assets/less/redash/query.less
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,13 @@ body.fixed-layout {
}
}

a.label-tag {
.label-tag {
background: fade(@redash-gray, 15%);
color: darken(@redash-gray, 15%);

&:hover {
&:hover,
&:focus,
&:active {
color: darken(@redash-gray, 15%);
background: fade(@redash-gray, 25%);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, { useMemo } from "react";
import { first, includes } from "lodash";
import Menu from "antd/lib/menu";
import Link from "@/components/Link";
import PlainButton from "@/components/PlainButton";
import HelpTrigger from "@/components/HelpTrigger";
import CreateDashboardDialog from "@/components/dashboards/CreateDashboardDialog";
import { useCurrentRoute } from "@/components/ApplicationArea/Router";
Expand All @@ -15,8 +16,8 @@ import AlertOutlinedIcon from "@ant-design/icons/AlertOutlined";
import PlusOutlinedIcon from "@ant-design/icons/PlusOutlined";
import QuestionCircleOutlinedIcon from "@ant-design/icons/QuestionCircleOutlined";
import SettingOutlinedIcon from "@ant-design/icons/SettingOutlined";

import VersionInfo from "./VersionInfo";

import "./DesktopNavbar.less";

function NavbarSection({ children, ...props }) {
Expand Down Expand Up @@ -129,9 +130,9 @@ export default function DesktopNavbar() {
)}
{canCreateDashboard && (
<Menu.Item key="new-dashboard">
<a data-test="CreateDashboardMenuItem" onMouseUp={() => CreateDashboardDialog.showModal()}>
<PlainButton data-test="CreateDashboardMenuItem" onClick={() => CreateDashboardDialog.showModal()}>
New Dashboard
</a>
</PlainButton>
</Menu.Item>
)}
{canCreateAlert && (
Expand Down Expand Up @@ -182,9 +183,9 @@ export default function DesktopNavbar() {
)}
<Menu.Divider />
<Menu.Item key="logout">
<a data-test="LogOutButton" onClick={() => Auth.logout()}>
<PlainButton data-test="LogOutButton" onClick={() => Auth.logout()}>
Log out
</a>
</PlainButton>
</Menu.Item>
<Menu.Divider />
<Menu.Item key="version" role="presentation" disabled className="version-info">
Expand Down
14 changes: 11 additions & 3 deletions client/app/components/CreateSourceDialog.jsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React from "react";
import PropTypes from "prop-types";
import { isEmpty, toUpper, includes, get } from "lodash";
import Button from "antd/lib/button";
import PlainButton from "@/components/PlainButton";
import List from "antd/lib/list";
import Modal from "antd/lib/modal";
import Input from "antd/lib/input";
import Steps from "antd/lib/steps";
import Button from "antd/lib/button";
import { wrap as wrapDialog, DialogPropType } from "@/components/DialogWrapper";
import Link from "@/components/Link";
import { PreviewCard } from "@/components/PreviewCard";
Expand Down Expand Up @@ -132,11 +133,12 @@ class CreateSourceDialog extends React.Component {
renderItem(item) {
const { imageFolder } = this.props;
return (
<List.Item className="p-l-10 p-r-10 clickable" onClick={() => this.selectType(item)}>
<List.Item className="p-l-10 p-r-10">
<PreviewCard
title={item.name}
imageUrl={`${imageFolder}/${item.type}.png`}
roundedImage={false}
onClick={() => this.selectType(item)}
data-test="PreviewItem"
data-test-type={item.type}>
<i className="fa fa-angle-double-right" />
Expand Down Expand Up @@ -180,7 +182,13 @@ class CreateSourceDialog extends React.Component {
<div data-test="CreateSourceDialog">
<Steps className="hidden-xs m-b-10" size="small" current={currentStep} progressDot>
{currentStep === StepEnum.CONFIGURE_IT ? (
<Step title={<a>Type Selection</a>} className="clickable" onClick={this.resetType} />
<Step
title={
<PlainButton type="link" onClick={this.resetType}>
Type Selection
</PlainButton>
}
/>
) : (
<Step title="Type Selection" />
)}
Expand Down
17 changes: 6 additions & 11 deletions client/app/components/EditInPlace.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React from "react";
import PropTypes from "prop-types";
import cx from "classnames";
import Input from "antd/lib/input";
import PlainButton from "@/components/PlainButton";

export default class EditInPlace extends React.Component {
static propTypes = {
Expand Down Expand Up @@ -66,18 +67,12 @@ export default class EditInPlace extends React.Component {
};

renderNormal = () =>
this.props.value ? (
<span
role="presentation"
onFocus={this.startEditing}
onClick={this.startEditing}
className={this.props.isEditable ? "editable" : ""}>
{this.props.value}
</span>
this.props.isEditable ? (
<PlainButton onClick={this.startEditing} onFocus={this.startEditing} className="editable">
{this.props.value || this.props.placeholder}
</PlainButton>
) : (
<a className="clickable" onClick={this.startEditing}>
{this.props.placeholder}
</a>
<span>{this.props.value}</span>
);

renderEdit = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ import React from "react";
import PropTypes from "prop-types";
import Dropdown from "antd/lib/dropdown";
import Menu from "antd/lib/menu";
import Button from "antd/lib/button";
import PlainButton from "@/components/PlainButton";
import { clientConfig } from "@/services/auth";
import Button from "antd/lib/button";

import PlusCircleFilledIcon from "@ant-design/icons/PlusCircleFilled";
import ShareAltOutlinedIcon from "@ant-design/icons/ShareAltOutlined";
Expand All @@ -18,16 +19,18 @@ export default function QueryControlDropdown(props) {
<Menu>
{!props.query.isNew() && (!props.query.is_draft || !props.query.is_archived) && (
<Menu.Item>
<a target="_self" onClick={() => props.openAddToDashboardForm(props.selectedTab)}>
<PlainButton onClick={() => props.openAddToDashboardForm(props.selectedTab)}>
<PlusCircleFilledIcon /> Add to Dashboard
</a>
</PlainButton>
</Menu.Item>
)}
{!clientConfig.disablePublicUrls && !props.query.isNew() && (
<Menu.Item>
<a onClick={() => props.showEmbedDialog(props.query, props.selectedTab)} data-test="ShowEmbedDialogButton">
<PlainButton
onClick={() => props.showEmbedDialog(props.query, props.selectedTab)}
data-test="ShowEmbedDialogButton">
<ShareAltOutlinedIcon /> Embed Elsewhere
</a>
</PlainButton>
</Menu.Item>
)}
<Menu.Item>
Expand Down
5 changes: 3 additions & 2 deletions client/app/components/FavoritesControl.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from "react";
import PropTypes from "prop-types";
import PlainButton from "@/components/PlainButton";

export default class FavoritesControl extends React.Component {
static propTypes = {
Expand Down Expand Up @@ -29,12 +30,12 @@ export default class FavoritesControl extends React.Component {
const icon = item.is_favorite ? "fa fa-star" : "fa fa-star-o";
const title = item.is_favorite ? "Remove from favorites" : "Add to favorites";
return (
<a
<PlainButton
title={title}
className="favorites-control btn-favorite"
onClick={event => this.toggleItem(event, item, onChange)}>
<i className={icon} aria-hidden="true" />
</a>
</PlainButton>
);
}
}
7 changes: 4 additions & 3 deletions client/app/components/HelpTrigger.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import cx from "classnames";
import Tooltip from "antd/lib/tooltip";
import Drawer from "antd/lib/drawer";
import Link from "@/components/Link";
import PlainButton from "@/components/PlainButton";
import CloseOutlinedIcon from "@ant-design/icons/CloseOutlined";
import BigMessage from "@/components/BigMessage";
import DynamicComponent, { registerComponent } from "@/components/DynamicComponent";
Expand Down Expand Up @@ -202,9 +203,9 @@ export function helpTriggerWithTypes(types, allowedDomains = [], drawerClassName
</Tooltip>
)}
<Tooltip title="Close" placement="bottom">
<a onClick={this.closeDrawer}>
<CloseOutlinedIcon />
</a>
<PlainButton onClick={this.closeDrawer}>
<CloseOutlinedIcon aria-hidden="true" />
</PlainButton>
</Tooltip>
</div>

Expand Down
3 changes: 2 additions & 1 deletion client/app/components/HelpTrigger.less
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@
border: 2px solid @help-doc-bg;
display: flex;

a {
a,
button {
height: 26px;
width: 26px;
display: flex;
Expand Down
26 changes: 0 additions & 26 deletions client/app/components/Link.jsx

This file was deleted.

35 changes: 35 additions & 0 deletions client/app/components/Link.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import React from "react";
import Button, { ButtonProps as AntdButtonProps } from "antd/lib/button";

interface LinkProps extends Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, "role"> {
href: string;
}

interface ButtonProps extends AntdButtonProps {
href: string;
}

function DefaultLinkComponent({ children, ...props }: React.AnchorHTMLAttributes<HTMLAnchorElement>) {
return <a {...props}>{children}</a>;
}

Link.Component = DefaultLinkComponent;

function Link({ tabIndex = 0, ...props }: LinkProps) {
return <Link.Component tabIndex={tabIndex} {...props} />;
}

// Ant Button will render an <a> if href is present.
function DefaultButtonLinkComponent(props: ButtonProps) {
return <Button {...props} />;
}

ButtonLink.Component = DefaultButtonLinkComponent;

function ButtonLink(props: ButtonProps) {
return <ButtonLink.Component {...props} />;
}

Link.Button = ButtonLink;

export default Link;
Loading