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

feat(vwc-accordion): viv-388 accordion #683

Merged
merged 41 commits into from
May 23, 2021
Merged

Conversation

JoelGraham93
Copy link
Contributor

@JoelGraham93 JoelGraham93 commented Feb 26, 2021

Currently doesn't support nested expansion panels which will be required for navigation

@github-actions
Copy link

🚀

Latest successful build of the PR deployed here.

🚀

return opened;
}

openAll(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a case where openAll is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only think this may be useful if it's a requirement to open all panels in desktop view but not on mobile.
I've messaged Joella to discuss if it's necessary

Copy link
Contributor

@yinonov yinonov Mar 1, 2021

Choose a reason for hiding this comment

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

just thinking it might be a property rather then a method. since a method will need to be called and if transitioned, can cause layout shift upon page render

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I've changed openAll to a prop 8b0d1be.
Initially I took inspiration from here where openAll is a method. But this doesn't look like a common approach and I can't think why we would want to open all expansion panels after a render.

Comment on lines 1 to 13
import { LitElement, property } from 'lit-element';
import { VWCExpansionPanelBase } from './vwc-expansion-panel-base.js';

export abstract class VWCExpansionPanelListBase extends LitElement {
@property({ type: Boolean, reflect: true })
multi = false;

private expansionPanels: HTMLCollectionOf<VWCExpansionPanelBase> | undefined = undefined;

constructor() {
super();
this.addEventListener('opened', this.handleOpened);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

using base file for component is probably not a standard we want to carry out from MWC, if it's not being used by other components and is not a common class. I know it's the same with badge, but following the badge case we decided it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any possibility this base class will be useful for navigation? e.g. collapsing list-expansion-panel when another is opened

Copy link
Contributor

Choose a reason for hiding this comment

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

yes actually. good point

@yinonov
Copy link
Contributor

yinonov commented Mar 30, 2021

@JoelGraham93 is this still a draft?

@JoelGraham93 JoelGraham93 marked this pull request as ready for review March 31, 2021 06:14
@yinonov
Copy link
Contributor

yinonov commented Apr 25, 2021

@JoelGraham93 JoelGraham93 changed the title feat(vwc-expansion-panel-list): viv-388 expansion panel list draft feat(vwc-accordion): viv-388 accordion draft Apr 27, 2021
@JoelGraham93 JoelGraham93 changed the title feat(vwc-accordion): viv-388 accordion draft feat(vwc-accordion): viv-388 accordion May 4, 2021
@JoelGraham93
Copy link
Contributor Author

Resolved conflicts with latest expansion-panel updates.
This PR includes the following suggestions from #740:

  • default to trailing toggle and and prop for leading toggle.
  • replace chevronToggle with IndicatorIconSets to easily allow future sets.
  • prevent leading icon from overriding leading toggle.

Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

do we want to observe multi property? in a use-case where the prop is removed and no longer allows multiple open panels. the ones that are already opened remain open. not sure which ones to close in such case...

@JoelGraham93
Copy link
Contributor Author

do we want to observe multi property? in a use-case where. the prop is removed and no longer allows multiple open panels. the once that are already opened remain open. not sure which ones to close in such case. all the latters?

Interesting point. @jshenkman does this sound like a potential requirement to remove mutli under certain circumstances? Example a viewport size change.
I wonder if it will be confusing for the user to have expansion panels close after they have already opened them

@yinonov
Copy link
Contributor

yinonov commented May 20, 2021

do we want to observe multi property? in a use-case where. the prop is removed and no longer allows multiple open panels. the once that are already opened remain open. not sure which ones to close in such case. all the latters?

Interesting point. @jshenkman does this sound like a potential requirement to remove mutli under certain circumstances? Example a viewport size change.
I wonder if it will be confusing for the user to have expansion panels close after they have already opened them

@jshenkman

@sonarcloud
Copy link

sonarcloud bot commented May 21, 2021

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
8.2% 8.2% Duplication

@yinonov yinonov merged commit 8b597ab into master May 23, 2021
@JoelGraham93 JoelGraham93 deleted the viv-388-expansion-panel-list branch May 23, 2021 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants