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.

Sign up for GitHub

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

Fix/url modal #466

Merged
merged 9 commits into from
Feb 22, 2024
Merged

Fix/url modal #466

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cbd337f
remove right justify
meganrm Feb 15, 2024
c303840
make sure all modals are getting consistent button styling
meganrm Feb 15, 2024
827db58
fix hover issues
meganrm Feb 15, 2024
06563c4
keep close x from disappearing
meganrm Feb 15, 2024
1d17a0b
change color vars to semantic ones
meganrm Feb 15, 2024
201caa4
make share target bigger
meganrm Feb 15, 2024
283013f
use semantic var name
meganrm Feb 15, 2024
277e9a4
fix typo due to copy paste
meganrm Feb 21, 2024
8a5c34c
revert change
meganrm Feb 22, 2024
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
9 changes: 6 additions & 3 deletions src/components/CustomModal/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,9 @@
border: 1px solid var(--dark-three) !important;
}

.modal :global(.ant-input) {
border: 1px solid var(--heather);
.modal :global(.ant-input-affix-wrapper) {
border: 1px solid var(--input-border-highlight);
border-radius: 3px;
text-align: right;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the main offender

}

.modal :global(.ant-divider) {
Expand All @@ -52,3 +51,7 @@
left: 0;
background-color: var(--heather);
}

.modal :global(.ant-modal-close):hover{
color: var(--white-three);
}
14 changes: 11 additions & 3 deletions src/components/FileUploadModal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import LocalFileUpload from "../LocalFileUpload";
import uploadFiles from "./upload-local-files";

import styles from "./style.css";
import theme from "../theme/light-theme.css";

const enum UploadTab {
// this version of antd requires tab keys to be strings
Expand Down Expand Up @@ -97,16 +98,23 @@ const FileUploadModal: React.FC<FileUploadModalProps> = ({

const footerButtons = (
<>
<Button onClick={closeModal}>Cancel</Button>
<Button type="primary" disabled={disableLoad} onClick={onLoadClick}>
<Button className="secondary-button" onClick={closeModal}>
Cancel
</Button>
<Button
type="primary"
className="primary-button"
disabled={disableLoad}
onClick={onLoadClick}
>
Load
</Button>
</>
);

return (
<CustomModal
className={styles.uploadModal}
className={[styles.uploadModal, theme.lightTheme].join(" ")}
title="Choose a Simularium file to load"
open
footer={footerButtons}
Expand Down
5 changes: 0 additions & 5 deletions src/components/FileUploadModal/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@
margin-bottom: 0;
}

.upload-modal :global(.ant-input-affix-wrapper) {
border-radius: 3px;
border-color: var(--input-border-highlight);
}

.upload-modal :global(.ant-input-suffix svg) {
color: var(--light-theme-modal-supplemental-text);
}
Expand Down
4 changes: 3 additions & 1 deletion src/components/LocalFileUpload/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ const LocalFileUpload: React.FC<FileUploadProps> = ({
state: { localFile: true },
}}
>
{children || <Button>Select file</Button>}
{children || (
<Button className="secondary-button">Select file</Button>
)}
</Link>
</Upload>
);
Expand Down
19 changes: 19 additions & 0 deletions src/components/LocalFileUpload/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,30 @@
.file-upload :global(.ant-upload-list) {
min-height: 30px;
}
.file-upload :global(.ant-upload-list-item):hover,
.file-upload :global(.ant-upload-list-item-list-type-text):focus,
.file-upload :global(.ant-upload-list-item-list-type-text) :global(.ant-upload-list-item-info):hover {
background-color: transparent;
}

.file-upload :global(.ant-upload-list-item-card-actions-btn) {
border: none;
}

.file-upload :global(.ant-upload-list-item-card-actions-btn) {
opacity: initial;
}

.file-upload :global(.ant-upload-list-item-card-actions-btn) svg {
color: #d14040;
}

.file-upload :global(.ant-upload-list-text .ant-upload-list-item-name) {
padding: 0;
}

.file-upload :global(.ant-upload-span) {
justify-content: flex-start;
width: max-content;
}

3 changes: 1 addition & 2 deletions src/components/ShareTrajectoryButton/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const ShareTrajectoryButton = ({
const tooltipOffset = isDisabled ? [0, -30] : [0, -18];

return (
<div className={styles.container}>
<div className={styles.container} onClick={handleShare}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Accessibility note: prefer using button over div for onClick behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a little bit of a weird thing since this is wrapping another button. I think the Dom would have a problem with a button within a button? but I can try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, validateDOMNesting(...): <button> cannot appear as a descendant of <button>.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why the onClick got moved to the div?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if you try it out on staging the click area is tiny. I tried other fixes but this was the only thing that got the target to be the whole button

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay... maybe try adding a role attribute to signal that the div can be treated like a button? https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role

I opened up the Simularium website and realized the entire site doesn't seem to work well with tab navigation, so there may be bigger fish to fry...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

your questions made me dive deeper into what was causing the button to be unclickable and I realized it was the tooltip placement. when the tooltip opens it actually covers the target because it had been aligned over the top. So I reverted this change and fixed that on a separate branch.

{isSharing ? (
<div className={styles.overlay}>
<ShareTrajectoryModal
Expand All @@ -51,7 +51,6 @@ const ShareTrajectoryButton = ({
>
<Button
className={isDisabled ? styles.disabled : undefined}
onClick={handleShare}
type="primary"
disabled={isDisabled}
>
Expand Down
4 changes: 4 additions & 0 deletions src/components/ShareTrajectoryButton/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,8 @@
height: calc(100vh - var(--header-height));
background-color: var(--black);
opacity: .7;
}

.container {
cursor: pointer;
}
4 changes: 2 additions & 2 deletions src/components/ShareTrajectoryModal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from "react";
import classNames from "classnames";
import { connect } from "react-redux";
import { Button, Checkbox, Divider, Input } from "antd";

Expand All @@ -11,7 +12,6 @@ import CustomModal from "../CustomModal";
import { Link, Warn } from "../Icons";
import { URL_PARAM_KEY_TIME } from "../../constants";
import { editUrlParams } from "../../util";
import classNames from "classnames";

import styles from "./style.css";
import theme from "../theme/light-theme.css";
Expand Down Expand Up @@ -166,7 +166,7 @@ const ShareTrajectoryModal = ({
</>
),
footer: (
<Button type="default" onClick={closeModal}>
<Button className="secondary-button" onClick={closeModal}>
Close
</Button>
),
Expand Down
49 changes: 33 additions & 16 deletions src/components/theme/light-theme.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,42 @@
/* Default and primary button colors differ within modals */
.light-theme :global(.ant-btn-default) {
background-color: var(--light-button-default-background);
border-color: var(--light-theme-modal-btn-default-color);
color: var(--light-button-default-color);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to match this styling

/* primary button */
.light-theme :global(.primary-button),
modal :global(.primary-button):focus {
border: 1px solid var(--light-theme-button-primary-bg);
border-radius: 3px;
background-color: var(--light-theme-button-primary-bg);
color: var(--light-theme-button-primary-text);
}

.light-theme :global(.primary-button):hover {
color: var(--light-theme-button-primary-text-hove);
border: 1px solid var(--light-theme-button-primary-border-hover);
}

.light-theme :global(.ant-btn-primary) {
background-color: var(--light-theme-btn-primary-bg);
border: var(--light-button-primary-border);
color: var(--light-theme-btn-primary-color);
.light-theme :global(.primary-button):disabled {
border: 1px solid var(--heather);
color: var(--light-theme-button-primary-text-disabled);
background-color: var(--light-theme-button-primary-bg-disabled);
}

.light-theme :global(.ant-btn-primary[disabled]) {
background: var(--light-button-disabled-background);
border: var(--light-button-disabled-border);
color: var(--light-theme-btn-primary-color);
/* secondary button */
.light-theme :global(.secondary-button),
modal :global(.secondary-button):focus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is modal supposed to be an element instead of a class or pseudo-class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, the was a artifact from moving these from another style sheet

border: 1px solid var(--light-theme-button-secondary-border);
border-radius: 3px;
background-color: transparent;
color: var(--light-theme-button-secondary-text);
}

.light-theme :global(.ant-btn-text) {
background-color: var(--dark-four);
border-color: var(--dark-four);
color: var(--white-three);
.light-theme :global(.secondary-button):hover {
background-color: var(--light-theme-button-secondary-bg-hover);
color: var(--light-theme-button-secondary-text-hover);
border: 1px solid var(--light-theme-button-secondary-border);
}

.light-theme :global(.secondary-button):disabled {
border: 1px solid var(--light-theme-button-primary-border-disabled);
color: var(--light-theme-button-secondary-text-disabled);
background-color: transparent;
}
19 changes: 18 additions & 1 deletion src/styles/colors.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
--footer-text: var(--warm-gray);
--footer-text-border-bottom: var(--greyish-brown);
--footer-link: var(--blue);
--input-border-highlight: var(--baby-purple);
--input-border-highlight: var(--heather);
--dark-theme-primary-color: var(--baby-purple);
--dark-theme-body-bg: var(--black);
--dark-theme-header-bg: var(--dark-three);
Expand Down Expand Up @@ -66,6 +66,23 @@
--dark-theme-version-badge-purple: var(--dark-five);
--light-theme-background-purple: var(--pale-grey);
--light-theme-button-purple: var(--dark-blue-grey);

--light-theme-button-primary-text: var(--white-three);
--light-theme-button-primary-bg: var(--dark-blue-grey);
--light-theme-button-primary-text-hover: var(--baby-purple);
--light-theme-button-primary-border-hover: var(--dark-four);
--light-theme-button-primary-text-disabled: var(--white-three);
--light-theme-button-primary-bg-disabled: var(--heather);


--light-theme-button-secondary-text: var(--dark-four);
--light-theme-button-secondary-border: var(--dark-four);
--light-theme-button-secondary-text-hover: var(--baby-purple);
--light-theme-button-secondary-bg-hover: var(--charcoal-grey);
--light-theme-button-secondary-text-disabled: var(--heather);
--light-theme-button-primary-border-disabled: var(--heather);


--light-theme-heading1-gray: var(--dark-three);
--light-theme-heading2-gray: var(--greyish-brown);
--light-theme-panel-light-gray: var(--white-four);
Expand Down
Loading