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(styles): add display property for custom HTML elements #12050

Merged
merged 7 commits into from
Oct 5, 2022
Merged

fix(styles): add display property for custom HTML elements #12050

merged 7 commits into from
Oct 5, 2022

Conversation

Akshat55
Copy link
Contributor

@Akshat55 Akshat55 commented Sep 2, 2022

Closes #11579

Specify display CSS property for specific wrapper classes.

In Angular, we have custom tags <ibm-table></ibm-table> unlike React, which uses traditional HTML tags (div, span, etc).

This results in browsers not knowing what the display style property of the tag should be (block, inline, etc). Where applicable, we have assigned a carbon class to the host e.g. <ibm-table class="bx--data-table-content"></ibm-table>. But there are cases where those classes have not specified the display property, table is one of them. (Not assigning display: block breaks horizontal scrolling)

This change will allow custom HTML elements such as <ibm-table class="bx--data-table-content"></ibm-table> (in Angular) to instruct the browser to use specific display property instead of inline by default. It will also allow us (carbon-components-angular team) to not specify styles within the component template.

Changelog

New

  • Specified display property for wrapper classes of accordion item, table, & context menu components.

Testing / Reviewing

  1. Build the styles package
  2. Comment out or remove ALL styles in React package storybook styles.scss (packages/react/.storybook/styles.scss)
  3. Import the built styles in React package storybook styles file:
@import '../../styles/css/styles.css';
  1. Run storybook & verify if there are any visual changes to Accordion, Data Table, & Context Menu components.

I have reviewed React package code & verified via storybook to ensure these changes do not impact the package.

You can also verify if the display properties make sense to the element they are applied to, for example, .cds--accordion__item class is applied to list (<li></li>) which the browser assigns the default display property of list-item.

Another example would be, .cds--data-table-content. This class is applied to a <div>, which browsers assign block as the display value.

Hence, these changes should not impact React package but will greatly benefit custom HTML elements (like the one's used in carbon-components-angular).

@Akshat55 Akshat55 requested a review from a team as a code owner September 2, 2022 05:29
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

DCO Assistant Lite bot All contributors have signed the DCO.

@Akshat55
Copy link
Contributor Author

Akshat55 commented Sep 2, 2022

I have read the DCO document and I hereby sign the DCO.

@netlify
Copy link

netlify bot commented Sep 2, 2022

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c67244e
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/633dfaa60165ae0007268498
😎 Deploy Preview https://deploy-preview-12050--carbon-components-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 settings.

@netlify
Copy link

netlify bot commented Sep 2, 2022

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit c67244e
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/633dfaa6d4430300085d228b
😎 Deploy Preview https://deploy-preview-12050--carbon-elements.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 settings.

@tay1orjones tay1orjones requested review from tay1orjones and aagonzales and removed request for aledavila September 6, 2022 15:15
@tay1orjones
Copy link
Member

It would be good to run VRT against multiple browsers locally for this one. I anticipate that adding display: block may cause some visual issues we won't be able to catch just by eye.

@tay1orjones
Copy link
Member

@Akshat55 It appears Angular has addressed this via the --displayBlock=true flag.

It seems like that might be a more appropriate solution rather than duplicating the default display properties on the components here. Let me know what you think.

@Akshat55
Copy link
Contributor Author

@tay1orjones We are already using the approach listed, but this is applying inline styles to the element. This seems unprofessional as the same inline style property is applied to all instances of that component, as shown in the image below.

image

Another issue with inline styles is that if the users want to make changes to the style by applying a class to the element, then they will have to use !important to override.

We hope to achieve something similar to what Angular Material has done, where the display properties are defined in the styles. Now we can override the classes on our end, but since we are already applying the wrapper classes from carbon, it makes sense to apply this change directly to the styles package.

Also, I don't think this is duplicating the display property since that display property isn't there in the styles package by default. This ensures that the display property stays consistent regardless of the HTML element used.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

That makes sense, thanks @Akshat55!

@Akshat55
Copy link
Contributor Author

Akshat55 commented Sep 26, 2022

Thanks @tay1orjones!

We've discovered a few more, should I include them as well in this PR or should I create another PR? Specifically for setting display properties for UI-shell side navigation items (display: list-item).

@tay1orjones
Copy link
Member

@Akshat55 A separate PR would be great!

@tay1orjones
Copy link
Member

bump for review @abbeyhrt

@kodiakhq kodiakhq bot merged commit ab38f00 into carbon-design-system:main Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: Need to update styles to work with custom HTML elements
3 participants