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 expansion panel component #332

Merged
merged 52 commits into from
May 26, 2022
Merged

Add expansion panel component #332

merged 52 commits into from
May 26, 2022

Conversation

ddaribo
Copy link
Contributor

@ddaribo ddaribo commented Apr 13, 2022

Closes #177

@ddaribo ddaribo changed the title feat(exp-panel): initial commit Add expansion panel component Apr 13, 2022
@MonikaKirkova MonikaKirkova marked this pull request as ready for review April 28, 2022 09:51

private headerTemplate() {
return html`
<div
Copy link
Member

Choose a reason for hiding this comment

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

The ARIA design pattern wraps the button in a heading (must check if that's always reqruired), which this is missing. Also if we wrap it internally there's no control over the aria level which might be a bit of an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, in terms of accessibility - if there's no title/sub projected this will still render as an empty button that is still targetable by tab navigation which I'm not sure is ideal.

Speaking of, we used expansion panel in some scenarios without a header even and if such a scenario is also intended to be supported then the hardcoded header would not fit.

Tagging @sdimchevski as well for this and I'll look for the example without a header to add here so we can evaluate

Copy link
Member

Choose a reason for hiding this comment

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

Okay took a bit to dig up since this is (no longer?) embedded in the docs, but the first card in this sample is using an expansion panel:
https://www.infragistics.com/angular-demos/layouts/card-sample-4
Point being, it doesn't have a header and it's toggled by some external button that either need to be positioned by the Card layout or under the panel as in this case. So my question is would this be a case we should consider supporting?

Choose a reason for hiding this comment

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

@damyanpetev I don't know why this Card in the example is using an expansion panel. Expansion panels by design should have a header that acts as the control for expanding and collapsing. I believe you can have a card and implement your own collapsible mechanic through the use of button events but that's not in the scope of the expansion panel.

simeonoff
simeonoff previously approved these changes May 19, 2022
rkaraivanov
rkaraivanov previously approved these changes May 19, 2022
@ChronosSF
Copy link
Member

so who's going to remove the DO NOT MERGE label

@mmart1n mmart1n dismissed stale reviews from rkaraivanov and simeonoff via 7a30764 May 19, 2022 16:57
@simeonoff simeonoff requested review from YoannaIvanova and removed request for sdimchevski May 20, 2022 11:38
@rkaraivanov rkaraivanov merged commit a675896 into master May 26, 2022
@rkaraivanov rkaraivanov deleted the expansion-panel/feat-177 branch May 26, 2022 08:36
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.

Expansion panel component