-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: Move and style copyButton, add to active and past detour views #2852
base: main
Are you sure you want to change the base?
Conversation
@@ -89,7 +93,7 @@ | |||
} | |||
} | |||
|
|||
.l-diversion-page-panel__header .c-diversion-panel__back-button { | |||
.l-diversion-page-panel__header .c-diversion-panel__outline-button--back { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: why change the name of this button? IMO the "outline" info doesn't seem necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally right. I renamed at one point, assuming I'd use the --back
as an extension of outline button, as in these lines, but not needed. (Plus, I see those might need tweaking as well)
.c-diversion-panel__outline-button { | ||
height: 2rem; | ||
border-radius: 0.2rem; | ||
font-size: 0.875rem; | ||
padding: 0.375rem 0.5rem; | ||
|
||
&--copy { | ||
margin: 0.78125rem 0; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this? this seems like a size="sm"
property on the React Bootstrap <Button/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my gosh, I totally reinvented the wheel. Thanks!!
&--copy { | ||
margin: 0.78125rem 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this margin should be handled via the .align-items-baseline
class on the parent flex
} | ||
> | ||
<Button | ||
className="c-diversion-panel__outline-button c-diversion-panel__outline-button--copy icon-link" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between these two suggestions
- https://github.com/mbta/skate/pull/2852/files#r1806786402
- https://github.com/mbta/skate/pull/2852/files#r1806794925
I don't think most of the classes are needed?
className="c-diversion-panel__outline-button c-diversion-panel__outline-button--copy icon-link" | |
className="icon-link" |
Add yourself to the copy-button user group to see this on the active / past detour views, because I haven't yet done the copy logic.
The button will be visible on the View Draft Detour panel without being added to the user group.
Now depends on:
^ only because the copy change will prevent the button from being too cramped on the "View Draft Detour" page