Skip to content

Conversation

@shanbady
Copy link
Contributor

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/4717

Description (What does it do?)

Sets a min width of 100px for buttons

How can this be tested?

Checkout this branch and ensure all buttons such as the login button have a minimum width of 100px

@shanbady shanbady marked this pull request as ready for review June 27, 2024 17:38
@shanbady shanbady added the Needs Review An open Pull Request that is ready for review label Jun 27, 2024
@jonkafton
Copy link
Contributor

jonkafton commented Jun 27, 2024

We probably don't want the min width on action and and icon only buttons (e.g. on the LearningResourceCards), though they could use separate treatment from the main Button component, maybe their own component.

The action buttons have their own widths, so minWidth: "auto" in the actionStyles around L305 should fix.

image

vs

image

@jonkafton jonkafton added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Jun 27, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

See Jon's comment.

Please also take a look at the relevant storybook pages... e.g.,

just to make sure things look reasonable.

@shanbady
Copy link
Contributor Author

See Jon's comment.

Please also take a look at the relevant storybook pages... e.g.,

just to make sure things look reasonable.

thanks. I keep forgetting we have something like this

@shanbady
Copy link
Contributor Author

@jonkafton

just pushed a fix with your suggestion

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

There's a tab question for @steven-hatch.

@shanbady ... requested change: Could you fix the MenuButton to not use Button? Probably should just be a snowflake styled.button(...).

":disabled": {
cursor: "default",
},
minWidth: "100px",
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki Jun 27, 2024

Choose a reason for hiding this comment

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

Our tab component is based on Button (it uses tertiary buttons). So this increases the width, of tabs, too.

@steven-hatch Do we want tabs to have min-width 100px? I can see pros/cons

Screenshot 2024-06-27 at 4 24 31 PM Screenshot 2024-06-27 at 4 24 42 PM Screenshot 2024-06-27 at 4 25 01 PM Screenshot 2024-06-27 at 4 25 18 PM

Really the only con is width on mobile screens.

Separate issue

Screenshot 2024-06-27 at 4 29 45 PM

In the screenshots above, you can see the mobile menu button in top-left corner now has wrong width.

@shanbady I'm sorry this came up... Button should not have been used for that in the first place. (Bright side: with your change, probably less likely to be used incorrectly for stuff like this.)

But, could you fix it so we don't regress? Probably that menu button should be a "Snowflake". I would just apply the relevant styles to it. See figma: https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=3852-51162&m=dev

The code is in frontends/mit-open/src/page-components/Header/MenuButton.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be addressed now @ChristopherChudzicki

@shanbady shanbady added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Jun 28, 2024
Comment on lines 40 to 41
borderWidth: "1px",
borderColor: "currentcolor",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove these? They do nothing with borderStyle: "none".

In <Button /> they do have an effect, but then variant="text" set borderStyle to none, so they aren't relevant for that variant.

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

👍 Looks good. I would suggest removing the borderWith and borderColor on the menu button, as noted.

@shanbady shanbady merged commit 939eb55 into main Jun 28, 2024
@shanbady shanbady deleted the shanbady/default-button-width branch June 28, 2024 20:22
@odlbot odlbot mentioned this pull request Jul 1, 2024
17 tasks
@odlbot odlbot mentioned this pull request Jul 1, 2024
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants