fix: Removed fixed width constraint from Save button

[goldjee]

SUMMARY

Currently, "Save" button in SQL Lab has fixed width. So, longer labels are trimmed. This is an attempt to fix that.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

English / before:

English / after:

Russian / before:

Russian / after:

TESTING INSTRUCTIONS

Switch to any language that has long "Save" button translation. Russian, for example.

ADDITIONAL INFORMATION

• Has associated issue: Fixes SQL Lab - Save button doesn't change its size based on label length #29477
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[rusackas]

The Russian button obviously improved in the "after' screenshot, but is there a good way to do this so that the English button (or any language with a shorter string) doesn't take up extra space?

[goldjee]

The Russian button obviously improved in the "after' screenshot, but is there a good way to do this so that the English button (or any language with a shorter string) doesn't take up extra space?

I've found line 39 at ./superset-frontend/src/components/DropdownButton/index.tsx that sets the standard width at 120px. If I remove this, the button won't take up extra space. This seems correct, because it will prevent other parts of the UI from label trimming. However, I'm hesitant to proceed since I don't know if this width was specified by design guidelines, or if changing it will break something.

I could also try modifying the CSS of the Save button to reset its width to auto.

Which solution is preferable?

[goldjee]

I decided to roll with the second approach for now. Left core component as is and applied width: auto to the styled Save button itself. This is how it looks now:

[mistercrunch]

I think the reason why we were not using the default antd behavior in this area is because the SAVE button changes text dynamically when there's text selected as shown here, and we didn't want the button to change with when selecting/deselecting text.

Looks like your current width: auto overrides the larger fixed with. Seems the ideal situation would be to target the larger with to the specific button (instead of making all buttons on the component wider, and then overriding with auto). Can we do this instead?

[goldjee]

@mistercrunch, thanks for the feedback, but I'm not sure that I'm following you. Upon reviewing the behavior when text is selected, I noticed that it's actually the "Run" button that changes its label, not the "Save" button. As my changes were targeted specifically at the SaveDatasetActionButton (not the DropdownButton or Button), the behavior you described is intact.

Probably the reason for my confusion is that I see "Run" as a plain button, not a dropdown one. Could you please clarify how I can get to the state depicted on your screenshot?

[mistercrunch]

Sorry for the confusion, I thought some of the css you're changing here was around to support the fixed width on the "dynamically labeled" Run button, but looking at the code in more details now, it clearly is not the case.

Digging in - and that's not related to this PR directly, but I'm not sure why so much of this styling logic is in this file as opposed to simply being more vanilla antd, or simply living in the component library, but that's out of scope for this PR.

One question is whether the &:first-of-type { width: auto; } is necessary. Isn't that the default antd behavior?

[goldjee]

One question is whether the &:first-of-type { width: auto; } is necessary. Isn't that the default antd behavior?

It seems that this is default indeed. I've tried to omit setting explicit auto width, but the button turned 120px wide. Looks like it's some sort of customization of Superset's UI kit.

I've found line 39 at ./superset-frontend/src/components/DropdownButton/index.tsx that sets the standard width at 120px.

[mistercrunch]

Yup here's a link to it for reference. https://github.com/apache/superset/blob/master/superset-frontend/src/components/DropdownButton/index.tsx#L38

Curious why it's there in the first place, I'd almost advise to delete that line to see what happens. Should be min-width if anything, but I'd say drop that line and we can fix whatever it breaks later if/where needed in a more targeted/proper way, along with a comment that expalins why it's ther (ie // added this because of {intricate use case})`

[goldjee]

@mistercrunch

Done!

[mistercrunch]

/testenv up

[goldjee]

Can I do something more for this PR to move forward?

[mistercrunch]

LGTM

[mistercrunch]

Approved, but looks like you're going to need to rebase to quick off the new required CI checks...

[mistercrunch]

I wish we had something like this -> isaacs/github#88

[goldjee]

Rebased

[mistercrunch]

/testenv up

[mistercrunch]

I think someone fixed the ephemeral environments, testing it here for giggles, will merge after confirming they work