Skip to content

Conversation

crisbeto
Copy link
Member

Currently the styles for the MDC-based list are included globally which causes conflicts with other list-based components like mat-select and mat-autocomplete. This can be seen by going to the MDC list demo and then the MDC select demo, and observing that the option text is now truncated.

These changes resolve the issue by nesting the styles under the mat-mdc-list-base class.

… list-based components

Currently the styles for the MDC-based list are included globally which causes conflicts with other list-based components like `mat-select` and `mat-autocomplete`. This can be seen by going to the MDC list demo and then the MDC select demo, and observing that the option text is now truncated.

These changes resolve the issue by nesting the styles under the `mat-mdc-list-base` class.
@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Apr 25, 2021
@crisbeto crisbeto requested review from devversion and mmalerba April 25, 2021 13:15
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 25, 2021
host: {
'class': 'mat-mdc-list mat-mdc-list-base mdc-deprecated-list',
'class': 'mat-mdc-list mat-mdc-list-base',
Copy link
Member

Choose a reason for hiding this comment

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

Is there a better way we can avoid style conflicts? I think this makes it a bit inconvenient in terms of instantiating the MDCList foundation, and declaring the adapter. I'm not sure if this currently has any a11y impact.

export function getInteractiveListAdapter(

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed that this isn't great, but the alternative would be to fix it in the select, autocomplete and menu specifically, but then it could still conflict with other list-based components in the future or other non-Material components the user might have on the page.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably change the adapter to apply any classes, attributes, etc to the div. The only other way I can think to do this is if MDC changed their mixins to allow us to add to the root selector:

@mixin deprecated-without-ripple(..., $root: '') {
  #{$root}.mdc-deprecated-list {
    ...
  }
}

Copy link
Member

@devversion devversion Apr 26, 2021

Choose a reason for hiding this comment

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

@mmalerba Yeah, I thought that too, but then realized that this could break a11y because the MDCList "root" usually receives the aria- attributes from the foundation. Personally I'd prefer what you showed. We might be able to convince MDC to support this because list is commonly used in various other components too, and it makes style isolation easier.

@crisbeto crisbeto added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Apr 26, 2021
@crisbeto
Copy link
Member Author

crisbeto commented Jul 9, 2021

Closing this since the concerns are valid and landing it will be tricky. I'll fix the issues in the specific components instead.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked This issue is blocked by some external factor, such as a prerequisite PR cla: yes PR author has agreed to Google's Contributor License Agreement P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants