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.

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

Add .iui-field base class for inputs #1919

Merged
merged 100 commits into from
May 2, 2024
Merged

Add .iui-field base class for inputs #1919

merged 100 commits into from
May 2, 2024

Conversation

FlyersPh9
Copy link
Collaborator

@FlyersPh9 FlyersPh9 commented Mar 13, 2024

Changes

  • Add .iui-field class which gives base styling (background-color, color, border-color, data-iui-variant, etc) for many inputs (<button>, <input>, <textarea>, <select>). This reduces duplicate CSS and helps keep styling consistent.
  • <input>, <textarea>, <select> now have matching hover state with <button>.
  • <input>, <textarea>, <select> text styling is different:
    • Disabled text is grayed out rather than not.
    • Disabled placeholder text no longer appears, otherwise it matches too closely to disabled text.

Testing

  • CSS test images.
  • React test images.

Docs

  • Changesets

After PR TODOs

  • When an input is disabled, don't show placeholder text because it is too similar to disabled value text color.

@FlyersPh9 FlyersPh9 self-assigned this Mar 13, 2024
@FlyersPh9
Copy link
Collaborator Author

FlyersPh9 commented Mar 15, 2024

@mayank99 I continued working on this yesterday and I'm up to 22 files changed locally (haven't pushed yet), starting to be concerned that the scope has creeped too far:

  • Button mixins have been removed so all files other than button.scss have been deleted.
  • The removal of these mixins caused having to make changes in breadcrumbs, pagination, etc. because they used these mixins.

@mayank99
Copy link
Contributor

I think it makes sense to keep the PRs separate, to make the review easier.

  1. This PR changes the class names. (CSS output)
  2. One PR removes the mixins from buttons. (internal Sass refactor)

You can even chain one PR to the other one, by creating a branch on top of it and changing the base. (Read more: 1, 2)

@FlyersPh9 FlyersPh9 added the visual change Requires the Figma library to be updated label Mar 18, 2024
<button class='iui-button' data-iui-variant='borderless' aria-label='Close'>
<svg-close-small class='iui-button-icon' aria-hidden='true'></svg-close-small>
</button>
<div class='iui-input-flex-container'>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated to this PR, but fixes a broken test.

data-iui-variant='borderless'
data-iui-size='small'
data-iui-active='true'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated to this PR, but fixes a button issue.

@FlyersPh9 FlyersPh9 marked this pull request as ready for review March 26, 2024 20:12
@FlyersPh9 FlyersPh9 requested review from a team as code owners March 26, 2024 20:12
@FlyersPh9 FlyersPh9 removed the request for review from a team March 26, 2024 20:12
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

LGTM!

I would maybe wait for another approval though, just to make sure nothing was missed.

cc @Ben-Pusey-Bentley

Copy link
Contributor

@Ben-Pusey-Bentley Ben-Pusey-Bentley left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@r100-stack r100-stack left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Just a few more comments of which some are nit/optional.

apps/css-workshop/src/components/Button.astro Outdated Show resolved Hide resolved
.changeset/lazy-seahorses-share.md Outdated Show resolved Hide resolved
.changeset/cyan-candles-walk.md Outdated Show resolved Hide resolved
.changeset/chilled-dolls-promise.md Outdated Show resolved Hide resolved
@mayank99 mayank99 merged commit c0b8f81 into main May 2, 2024
16 checks passed
@mayank99 mayank99 deleted the jon/field-class branch May 2, 2024 18:52
@imodeljs-admin imodeljs-admin mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
visual change Requires the Figma library to be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants