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

chore: make the milestone status a button #9255

Merged
merged 2 commits into from
Feb 7, 2025
Merged

chore: make the milestone status a button #9255

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb377f9
chore: make the milestone status a button
nunogois Feb 7, 2025
e60385c
refactor: use a plain html button
nunogois Feb 7, 2025
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
95 changes: 54 additions & 41 deletions ...atureView/FeatureOverview/ReleasePlan/ReleasePlanMilestone/ReleasePlanMilestoneStatus.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,48 @@
import { Link, styled } from '@mui/material';
import { Button, styled } from '@mui/material';
import PlayCircleIcon from '@mui/icons-material/PlayCircle';
import PauseCircleIcon from '@mui/icons-material/PauseCircle';
import TripOriginIcon from '@mui/icons-material/TripOrigin';

export type MilestoneStatus = 'not-started' | 'active' | 'paused' | 'completed';

const StyledStatus = styled('div', {
const StyledStatusButton = styled(Button, {
shouldForwardProp: (prop) => prop !== 'status',
})<{ status: MilestoneStatus }>(({ theme, status }) => ({
display: 'flex',
alignItems: 'center',
gap: theme.spacing(1),
paddingRight: theme.spacing(1),
fontSize: theme.fontSizes.smallerBody,
borderRadius: theme.shape.borderRadiusMedium,
backgroundColor:
status === 'active' ? theme.palette.success.light : 'transparent',
color:
status === 'active'
? theme.palette.success.contrastText
: status === 'completed'
? theme.palette.text.secondary
: theme.palette.text.primary,
'& svg': {
'&&&': {
'&:focus-visible': {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen this before: is this just a hack to override the specificity? In this case, why use a MUI button at all? Why not just use a regular button?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it is. How do you feel about this instead? e60385c

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I think that's better, but why use pointer-events: none? Wouldn't it be better to use cursor: default? You're already using the disabled attribute, which makes it unreachable by tab anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

This brings back our previous behavior of the button being "click-through" when disabled, which means that if the milestone is running or paused, and you click the disabled button, you still toggle the accordion. The cursor is still pointer because, again, we "click-through" the button and toggle the accordion.

Would you expect a different behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it. Yeah, I can see the logic there. I'm not sure it's what I'd expect, but if it's UX approved, then I don't really have any arguments against it. maybe that it's unintuitive? Do disabled buttons still get hover styles? Because it might be strange if it looks like it can be interacted with but it can't? It can't get tab focus, so that's not an issue though.

Copy link
Member Author

@nunogois nunogois Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do disabled buttons still get hover styles?

No, they don't. Those statuses act the same as before this PR.

I'm proceeding with this for now but we can revisit this later if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! My point was I think it'd be confusing if they did. So yeah, happy for you to merge it 😄

outline: `2px solid ${theme.palette.primary.main}`,
},
padding: theme.spacing(0, 0.5),
paddingRight: theme.spacing(1),
height: theme.spacing(3),
fontSize: theme.fontSizes.smallerBody,
lineHeight: theme.fontSizes.smallerBody,
fontWeight: theme.fontWeight.medium,
borderRadius: theme.shape.borderRadiusMedium,
color:
status === 'active'
? theme.palette.success.main
? theme.palette.success.contrastText
: status === 'paused'
? theme.palette.text.disabled
: status === 'completed'
? theme.palette.neutral.border
: theme.palette.primary.main,
? theme.palette.text.primary
: theme.palette.primary.main,
textDecoration:
status === 'not-started' || status === 'completed'
? 'underline'
: 'none',
'& svg': {
color:
status === 'active'
? theme.palette.success.main
: status === 'paused'
? theme.palette.text.disabled
: status === 'completed'
? theme.palette.neutral.border
: theme.palette.primary.main,
height: theme.spacing(3),
width: theme.spacing(3),
},
},
}));

Expand All @@ -52,27 +64,28 @@ export const ReleasePlanMilestoneStatus = ({
? 'Restart'
: 'Start';

const statusIcon =
status === 'active' ? (
<TripOriginIcon />
) : status === 'paused' ? (
<PauseCircleIcon />
) : (
<PlayCircleIcon />
);

const disabled = status === 'active' || status === 'paused';

return (
<StyledStatus status={status}>
{status === 'active' ? (
<TripOriginIcon />
) : status === 'paused' ? (
<PauseCircleIcon />
) : (
<PlayCircleIcon />
)}
{status === 'not-started' || status === 'completed' ? (
<Link
onClick={(e) => {
e.stopPropagation();
onStartMilestone();
}}
>
{statusText}
</Link>
) : (
<span>{statusText}</span>
)}
</StyledStatus>
<StyledStatusButton
status={status}
onClick={(e) => {
e.stopPropagation();
onStartMilestone();
}}
disabled={disabled}
startIcon={statusIcon}
>
{statusText}
</StyledStatusButton>
);
};
Loading