Replace PositronButton with Button#12795
Conversation
|
E2E Tests 🚀 |
|
When we were looking at this, we focused mostly on the React component, but there were also some css classes for Cursory search shows up custom css in:
These selectors will no longer match and styles no longer applied. |
I moved the `cursor: pointer` css into the chevron div from the parent div since the entire parent element isn't clickable. the cursor pointer is a bit misleading so I changed that while I was in here.
541dfe5 to
da1f7c4
Compare
The old PositronButton rendered a <div>, which is block-level and fills its parent's width by default. The new Button renders a <button>, which is inline-block and shrinks to fit its content. This caused the Edit button to collapse to text width. Copy and Cancel weren't affected because their container (.buttons .bottom) was already display: flex; flex-direction: column, which stretches children to full width. The Edit button's container (.buttons .top) wasn't a flex container, so the fix is to make it one to match.
|
Alrighty, I went through all the button cases and tested them + made any CSS changes needed to get them to render correctly. Still need to fix an issue in the ConsoleInstanceInfoPopup. The channel links aren't rendering in block order. |
The old PositronButton rendered a <div>, which is block-level and stacks vertically by default. The new Button renders a <button>, which is inline-block and sits side by side. This caused the output channel links in the console instance info popup to render horizontally instead of vertically. The fix makes .actions a flex column container so the links stack correctly.
nstrayer
left a comment
nstrayer
left a comment
There was a problem hiding this comment.
Looks good with one caveat around the split button component.
| justify-content: center; | ||
| padding: 0 4px; | ||
| border-left: 1px solid var(--vscode-widget-border); | ||
| cursor: pointer; |
There was a problem hiding this comment.
This causes the right side of these buttons to not show the pointer cursor as they are not a button component. Probably should be a <button/> though.
There was a problem hiding this comment.
The props are all the same as our Button component so I think we can just replace the div with Button!
| {children ?? label} | ||
| </PositronButton> | ||
| </Button> | ||
| <div |
There was a problem hiding this comment.
This could be converted to a Button as well but that's probably beyond this PR scope. Will need to cursor: pointer deletion reverted to keep it working fully though.
There was a problem hiding this comment.
I was able to convert this to a button!
samclark2015
left a comment
samclark2015
left a comment
There was a problem hiding this comment.
LGTM! Checked buttons in a variety of contexts, all behaved as expected 💯
6 participants
This PR migrates all use cases of
PositronButtonto our preferredButtoncomponent.PositronButtonrendered an unstyled<div>withrole="button". The newerButtoncomponent renders a proper<button>element with built-in hover management, focus handling, and accessibility. This migration improves semantic HTML and keyboard accessibility across all affected areas.Release Notes
New Features
Bug Fixes
QA Notes
@:connections @:console @:positron-notebooks @:plots
Console:
Connections:
render at full width and function correctly
Notebooks:
Plots: