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

[CRISP-683] Add <sp-dropdown /> #95

Merged
merged 18 commits into from
Aug 26, 2019
Merged

Conversation

Westbrook
Copy link
Contributor

@Westbrook Westbrook commented Aug 1, 2019

Description

Add support for the Dropdown pattern from Spectrum CSS: http://opensource.adobe.com/spectrum-css/2.13.0/docs/#dropdown

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Westbrook Westbrook mentioned this pull request Aug 1, 2019
10 tasks
Copy link
Collaborator

@cuberoot cuberoot left a comment

Choose a reason for hiding this comment

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

Great start. Thanks for working on this! We'll need it soon.

src/dropdown/dropdown.ts Outdated Show resolved Hide resolved
src/dropdown/dropdown.ts Outdated Show resolved Hide resolved
@Westbrook Westbrook force-pushed the westbrook/CRISP-683/dropdown branch 2 times, most recently from 1eacf98 to 306b39b Compare August 19, 2019 21:33
protected get buttonContent(): TemplateResult[] {
return [
html`
<svg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added #155 to track getting this icon (and related icons) into the project more smoothly in the future.

<slot></slot>
`}
</div>
${this.invalid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the sizing and spacing of the invalid and chevron icons are not 100% to Spectrum CSS spec, and I have created #156 to track making the uiicon styles available in this project.

@@ -29,4 +29,5 @@ button {

#selected {
flex-shrink: 0;
margin-top: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This workaround is related to #156 in that the spacing on the icon is not correct without this override until the uiicon styles are available.

this.setAttribute('role', 'menuitem');
public connectedCallback(): void {
super.connectedCallback();
if (!this.hasAttribute('role')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allow the usage to determine the role of the element.

},
});
this.dispatchEvent(queryRoleEvent);
this.setAttribute('role', queryRoleEvent.detail.role || 'menuitem');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make menuitem the default role.

@Westbrook Westbrook marked this pull request as ready for review August 23, 2019 19:09
Copy link
Collaborator

@andrewhatran andrewhatran left a comment

Choose a reason for hiding this comment

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

Looks good to me! :)

@Westbrook Westbrook merged commit 0f82e1e into master Aug 26, 2019
@Westbrook Westbrook deleted the westbrook/CRISP-683/dropdown branch August 26, 2019 17:48
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.

3 participants