Feature/custom button #625

interim17 · 2025-01-15T19:29:59Z

Time estimate or Size

small/medium
the final PR into the feature branch for this migration??

Problem

Button styling got messed up during ant migration

Also our approach to styling buttons is a bit all over the place.

Solution

Replace NavButton with more general CustomButton, a styled-component.

Delete: NavButton et. al, and light-theme.css and its directory.

The light-theme file was a bit of an anachronism from auto-conversion work.

`ButtonClass`

Is expanded to cover all five of our basic button styled from the figma master guide (light and dark primary, light and dark secondary, and action buttons). Using the enum instead of raw strings is more type-safe and developer friendly.
Native antd variant prop is omitted and replaced with our own custom variants.

`CustomButton`

As in the case of DropdownMenuItems, antd can be a bit heavy handed with class names, and also swaps out underlying html in certain cases.
Using a styled-component helps address specificity and consolidate the styling of buttons in one place.

Other buttons in the app (ViewportButton and DropdownMenuItems) are functionally and stylistically distinct, and either weren't much affected in migration or have already been addressed.

…rk with new versions

…um-website into feature/config-provider

… theme colors

…button styling

…um-website into feature/ant5-styling

…simularium-website into feature/custom-button

interim17 · 2025-01-15T19:32:18Z

src/components/CustomButton/index.tsx

+interface CustomButtonProps extends Omit<AntButtonProps, "type" | "variant"> {
+    variant?: ButtonClass;
+    titleText?: string;
+    icon?: ReactNode;
+    onClick?: () => void;
+}
+
+interface TooltipText {
+    defaultText: string;
+    disabledText?: string;
+}
+
+interface TooltilButtonProps extends CustomButtonProps {
+    tooltipText: TooltipText;
+    tooltipPlacement?: TooltipPlacement;
+}


Currently the style rules, basic button, and button with tooltip are all here in the same file. I'm open to the argument that they be split up! But at this stage I like having it all in one place.

ShrimpCryptid

Some minor comments but this approach is really elegant! LGTM!

ShrimpCryptid · 2025-01-16T22:47:57Z

src/components/LocalFileUpload/index.tsx

 import React from "react";
 import { Link } from "react-router-dom";
-import { Button, message, Upload, UploadProps } from "antd";
+import { message, Upload, UploadProps } from "antd";
 import { CloseOutlined } from "@ant-design/icons";
 import { RcFile } from "antd/lib/upload";

 import { ButtonClass } from "../../constants/interfaces";
 import { VIEWER_PATHNAME } from "../../routes";
+import { CustomButton } from "../CustomButton";


This is making me think it might be nice to add an import sorting plugin to the repo...

yes, I want that

ShrimpCryptid · 2025-01-16T22:51:41Z

src/styles/theme/colors.ts

    button: {
        hoverBg: baseColors.transparent,
        activeBg: baseColors.transparent,
+        primary: {
+            light: {
+                background: baseColors.dark.five,
+                text: baseColors.gray.six,
+                hover: {
+                    background: baseColors.dark.one,
+                    text: semanticColors.primaryPurple,
+                },
+                active: {
+                    background: baseColors.dark.one,
+                    text: semanticColors.primaryPurple,
+                },
+                focus: {
+                    outline: semanticColors.primaryPurple,
+                },
+                disabled: {
+                    background: baseColors.purple.three,
+                    text: baseColors.gray.six,
+                },
+            },


I'm a big fan of this nested structure! I should clean up TFE's theme type at some point.

ShrimpCryptid · 2025-01-16T22:55:02Z

src/components/CustomButton/index.tsx

+    disabledText?: string;
+}
+
+interface TooltilButtonProps extends CustomButtonProps {


Suggested change

interface TooltilButtonProps extends CustomButtonProps {

interface TooltipButtonProps extends CustomButtonProps {

ShrimpCryptid · 2025-01-16T22:58:59Z

src/components/CustomButton/index.tsx

+                }
+
+                &:active:not(:disabled) {
+                    background-color: ${hover.background};


Nit: I feel like I'd expect this to be active.background, though I can see why it would be redundant if none of the buttons change background between active and hover states.

ShrimpCryptid · 2025-01-16T23:02:49Z

src/components/CustomButton/index.tsx

+    const [tooltipRenderText, setTooltipRenderText] = useState(
+        disabled ? tooltipText.disabledText : tooltipText.defaultText
+    );


Does this need to be stateful? Is this to prevent the rendered text from changing while the tooltip is open?

Hey good catch, no it doesn't. Also the tooltip behavior wasn't actually working with the new disabled prop so this comment helped me catch that!

Adding a wrapper div to handle the mouse events so disabled buttons still show tooltips, and making tooltipRenderText a plain const since the button always re renders!

tyler-foster

Smells reasonable to me!

Left some minor comments. Not sure I'm up to speed with the latest CSS technology.

tyler-foster · 2025-01-27T21:10:20Z

src/styles/colors.css

-    --dark-theme-secondary-button-disabled-bg: transparent;
-    --dark-theme-secondary-button-disabled-border: var(--dark-theme-header-bg);
-    --dark-theme-secondary-button-disabled-color: var(--dim-gray-two);
+    /* --dark-theme-primary-button-bg: var(--baby-purple);


Are lines 60 and 61 here still necessary?

tyler-foster · 2025-01-27T21:12:47Z

src/constants/interfaces.ts

@@ -42,8 +42,10 @@ export type TooltipPlacement =
    | "rightBottom";

 export enum ButtonClass {


Too lazy to check this myself - is this enum used anywhere beyond the CustomButton component props? If not, would it be the right idea to just export it from CustomButton? That's what I would do, but I don't typically make big constant files like these.

tyler-foster · 2025-01-27T21:31:02Z

src/components/CustomButton/index.tsx

+            border: 1px solid ${variant === "primary" ? background : border};
+            color: ${text};
+
+            &&& {


Yeesh. Which brand of CSS magic does this triple ampersand belong to? Does this mean "parent-parent-parent"..?

No! It's both better and worse than that.

In styled-components this just directly raises the specificity of the current selector. Yes, this is arbitrary and smells a bit like using !important but for me its a needed workaround for quality of life when styling antd.

Because some antd components apply many different class names, change element nesting, and even changes which base html element is rendered for different components, it can be tricky to ensure that our styles will be consistently applied. This solves that in a brute force kind of way, but is better than using !important because these triple applied class hashes CAN be overridden by something of greater selector specificity, whereas !important ignores the specificity hierarchy entirely.

When our styles don't really map onto ant's opinions very well, I prefer to just sort of veto all their styling by using this triple ampersand.

Thanks, I sort of it get it now.

Does there come a point where we eschew the ant buttons altogether and just roll our own from scratch 🤔 ? I'd be curious as to when the team would consider that to be worth it.
This isn't a request for changes though! Feel free to resolve.

interim17 added 23 commits November 4, 2024 12:34

bump antd and typescript, and update build tools and components to wo…

ee6d0d7

…rk with new versions

consolidate ts.config

334431a

Merge branch 'feature/ant5' of https://github.com/simularium/simulari…

1c629d5

…um-website into feature/config-provider

replace ant-vars.less with ConfigProvider

ad19491

semantic color variables names for config provider

2a6b563

align css var colors and theme color variables, divide light and dark…

f24d313

… theme colors

consolidate theme variables

58c7ea1

styling fixes for antd5

24fd57e

fix record icon position

cd63f17

merge dropdown button changes

4cb5dec

pass onClick into dropdown menu items

eabc972

fix meta data panel font sizes

d2f0662

fix colorpicker alignment offset

68cbed4

remove unused commented style rule

e7b159c

remove unused themeConfig

857baef

apply styling fixes and use new CustomButton component to streamline …

c3e87d7

…button styling

bump viewer version

eaf84b3

replace clickHandler with onClick

603b4fa

a few more ant migration style fixes

c9c58b1

Merge branch 'feature/ant5' of https://github.com/simularium/simulari…

69cd8ef

…um-website into feature/ant5-styling

restore className on dropdown render div

7000cd9

Merge branch 'feature/ant5-styling' of https://github.com/simularium/…

625f288

…simularium-website into feature/custom-button

remove duplicate import

ca5eba0

interim17 commented Jan 15, 2025

View reviewed changes

interim17 added 2 commits January 15, 2025 11:35

remove unused css vars

54580dd

remove unused import

ace0823

interim17 marked this pull request as ready for review January 15, 2025 21:08

interim17 requested a review from a team as a code owner January 15, 2025 21:08

interim17 requested review from ShrimpCryptid and removed request for a team January 15, 2025 21:08

interim17 requested a review from tyler-foster January 15, 2025 21:08

ShrimpCryptid approved these changes Jan 16, 2025

View reviewed changes

use plain const for tooltip text on custom buttons

d504830

tyler-foster approved these changes Jan 27, 2025

View reviewed changes

Base automatically changed from feature/ant5-styling to feature/ant5 January 30, 2025 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/custom button #625

Feature/custom button #625

interim17 commented Jan 15, 2025 •

edited

Loading

interim17 Jan 15, 2025

ShrimpCryptid left a comment

ShrimpCryptid Jan 16, 2025

interim17 Jan 17, 2025

ShrimpCryptid Jan 16, 2025

ShrimpCryptid Jan 16, 2025

ShrimpCryptid Jan 16, 2025

ShrimpCryptid Jan 16, 2025

interim17 Jan 17, 2025

tyler-foster left a comment

tyler-foster Jan 27, 2025

tyler-foster Jan 27, 2025

tyler-foster Jan 27, 2025

interim17 Jan 28, 2025

tyler-foster Jan 28, 2025

	interface TooltilButtonProps extends CustomButtonProps {
	interface TooltipButtonProps extends CustomButtonProps {

		@@ -42,8 +42,10 @@ export type TooltipPlacement =
		\| "rightBottom";

		export enum ButtonClass {

Feature/custom button #625

Are you sure you want to change the base?

Feature/custom button #625

Conversation

interim17 commented Jan 15, 2025 • edited Loading

Time estimate or Size

Problem

Solution

ButtonClass

CustomButton

Choose a reason for hiding this comment

ShrimpCryptid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyler-foster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interim17 commented Jan 15, 2025 •

edited

Loading

`ButtonClass`

`CustomButton`