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

Tooling: Make sure that components are honoring a correct default display value. #2513

Closed
driskull opened this issue Jul 12, 2021 · 10 comments
Closed
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. help wanted Issues that the core team needs help with in a sprint. tooling Issues relating to build system fixes or improvements.

Comments

@driskull
Copy link
Member

Summary

I've noticed a few components that are by default displayed as inline but should probably be displayed as block by default.

For example, a new component calcite-list is displayed as inline by default. cc @asangma

calcite-alert is also displayed as inline by default.

It seems this is an issue with more than a few components.

Measure of Success

All components are displayed correctly by default.

Resources

Maybe we can add a common test to help make sure that the components are honoring a default display?

export async function displayedAs(componentTagOrHTML: TagOrHTML, display: string): Promise<void> {
  const page = await simplePageSetup(componentTagOrHTML);
  const element = await page.find(getTag(componentTagOrHTML));

  expect((await element.getComputedStyle()).display).toBe(display);
}

cc @jcfranco @julio8a can you please prioritize?

@driskull driskull added tooling Issues relating to build system fixes or improvements. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Jul 12, 2021
@jcfranco jcfranco added this to the Sprint 7/19 – 7/30 milestone Jul 12, 2021
@jcfranco jcfranco added help wanted Issues that the core team needs help with in a sprint. and removed needs triage Planning workflow - pending design/dev review. labels Jul 12, 2021
@jcfranco jcfranco self-assigned this Jul 12, 2021
@jcfranco jcfranco added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Jul 12, 2021
@driskull
Copy link
Member Author

Other components with issues:

  • dropdown-group
  • fab
  • graph?
  • inline editable?
  • label?
  • link
  • notice (this is inline by default but then flex when [active]). I think there are a few like this.

We should probably just have a test for every component I think.

@jcfranco
Copy link
Member

A few questions on this one.

  • Do we have a default/common display value?
  • Do we need to support more than inline | inline-block?

In any case, I think we could enhance the renders test helper to test the display.

renders("calcite-foo", { displayAs: "block" });

@driskull
Copy link
Member Author

Do we have a default/common display value?

No, I don't think so. It depends on the component.

Do we need to support more than inline | inline-block?

Yes, I know that some of our components are displayed as block, flex, or inline-flex by default. We should just keep them consistent once we decide and probably document their default display as well.

The problem I'm seeing at this time is that some do not set a display at all and by default that makes them inline. However, they have heights set and should not be set to display inline.

@driskull
Copy link
Member Author

@jcfranco I can take this one if you want.

@jcfranco
Copy link
Member

Thanks for tackling this one, @driskull!

I had asked if we had a default display based on our common component structure (I think most use an internal wrapper) to possibly define a default on the global CSS and define needed overrides in individual component stylesheets. If applicable, this could slightly reduce the file size while simplifying the pattern and test util. WDYT?

@driskull
Copy link
Member Author

Gotcha. I don't think we're supposed to define :host { styles on the global stylesheet right? We would have to target all the calcite- prefixed components.

It seems like we don't really have a default display anyway. Our "inline" components are displayed as either inline-block or inline-flex. Our "block" components are displayed as block or flex.

Maybe we could work to make these all flex and inline-flex or all block and inline-block? I think we're using flex/inline-flex to help style and position child components.

driskull added a commit that referenced this issue Jul 29, 2021
…y value (#2669)

* test: Make sure that components are honoring a correct default display value. #2513

* Allow component to be rendered without all props.

* fix tests + cleanup

* fix month header display
@driskull driskull added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Jul 29, 2021
@driskull
Copy link
Member Author

@asangma @jcfranco what do you think? Should we only have our components default display be flex and inline-flex for consistency?

Should we document the default display value (using a custom JS tag)?

@jcfranco
Copy link
Member

Even if we can't/won't do it on a calcite-global selector level, I think we could define the default on %component-host, right?

Should we only have our components default display be flex and inline-flex for consistency?

block has come up as the desired display default for custom elements on some W3C discussions. Interestingly, only inline and block display values are discussed.

Should we document the default display value (using a custom JS tag)?

A custom JSDoc tag would be useful.

@driskull
Copy link
Member Author

only inline and block display values are discussed.

Yeah that kind of makes sense since most native elements are either block or inline by default. Maybe we should follow.

That means any parent component would need to style slotted children to be flex.

@jcfranco
Copy link
Member

Verified locally!

@jcfranco jcfranco added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. help wanted Issues that the core team needs help with in a sprint. tooling Issues relating to build system fixes or improvements.
Projects
None yet
Development

No branches or pull requests

2 participants