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

fix(style) - housekeeping for button css #994

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Shashika6
Copy link
Collaborator

@Shashika6 Shashika6 commented Apr 28, 2024

This PR provides refactoring/housekeeping of the button component.

  • Replace explicit instances of ilo with the $prefix variable to enable theming later on
  • Make sure classnames conform to modified BEM convention (block--element__modifier)
  • Remove local variables that should instead use standard ones from the theme
  • Explain any local variables that have to be used for special cases
  • Remove unneeded rules
  • Replace map-get invocations with the name of the css variable exported by the themes package
  • Use LTR/RTL agnostic css (Example padding-inline-start instead of padding-left)
  • Where necessary, break up the CSS for very big components into logical sub-components, that could potentially be exported from the Design System by themselves

Related to #971

Design bugs fixed :-

Align the small button icon and label

Before :-
Screenshot 2024-05-06 at 16 57 33

After :-
Screenshot 2024-05-06 at 17 00 28

Fix icon only component in twig

Before :-
Screenshot 2024-05-06 at 17 02 44

After :-
Screenshot 2024-05-06 at 17 02 54

Copy link

changeset-bot bot commented Apr 28, 2024

🦋 Changeset detected

Latest commit: ef425c4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@ilo-org/react Patch
@ilo-org/styles Patch
@ilo-org/twig Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Apr 28, 2024

Deploy Preview for ilo-ui-twig-develop ready!

Name Link
🔨 Latest commit ef425c4
🔍 Latest deploy log https://app.netlify.com/sites/ilo-ui-twig-develop/deploys/663cc4d49d54630008bb1d69
😎 Deploy Preview https://deploy-preview-994--ilo-ui-twig-develop.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Apr 28, 2024

Deploy Preview for ilo-ui-twig ready!

Name Link
🔨 Latest commit ef425c4
🔍 Latest deploy log https://app.netlify.com/sites/ilo-ui-twig/deploys/663cc4d4b42bd60008f776b3
😎 Deploy Preview https://deploy-preview-994--ilo-ui-twig.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Apr 28, 2024

Deploy Preview for ilo-ui-react ready!

Name Link
🔨 Latest commit ef425c4
🔍 Latest deploy log https://app.netlify.com/sites/ilo-ui-react/deploys/663cc4d4dcab350007367f99
😎 Deploy Preview https://deploy-preview-994--ilo-ui-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Shashika6 Shashika6 self-assigned this Apr 28, 2024
@Shashika6
Copy link
Collaborator Author

@justintemps I noticed that there is a border radius token just used for the button. We should get rid of this one right?

  • The reason why I didn't delete it yet is because its been used in fileupload as well. Should we come back to this on file upload ?

packages/themes/tokens/radius/button.json

Copy link
Member

@justintemps justintemps left a comment

Choose a reason for hiding this comment

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

Hi @Shashika6 think you're still missing some things here

padding: 0;
@include borderradius("button");
border-radius: $radius-button-top-left $radius-button-top-right
Copy link
Member

Choose a reason for hiding this comment

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

@Shashika6 Do we need tokens for this? What do you think @GGKapanadze ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is used in like 6 components so I think it should be fine right what do you think @GGKapanadze

Copy link
Member

Choose a reason for hiding this comment

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

Of course, if it's button-specific we should define it on the button level but if it's general we can move it to the foundation after #999

packages/styles/scss/components/_button.scss Outdated Show resolved Hide resolved
packages/styles/scss/components/_button.scss Outdated Show resolved Hide resolved
packages/styles/scss/components/_button.scss Outdated Show resolved Hide resolved
packages/styles/scss/components/_button.scss Outdated Show resolved Hide resolved
packages/styles/scss/components/_button.scss Outdated Show resolved Hide resolved
@justintemps
Copy link
Member

@GGKapanadze could you give this a look?

@GGKapanadze
Copy link
Member

@justintemps @Shashika6 data-js-processed selector is removed and replaced with twig component attributes

Copy link
Member

@GGKapanadze GGKapanadze left a comment

Choose a reason for hiding this comment

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

Everything looks good, but we can hold it to integrate with #999

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants