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.

Sign up for GitHub

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

Jump to bottom

feat(segmented-button): add adapters and foundations #6165

Merged
merged 13 commits into from
Jul 8, 2020
Merged

feat(segmented-button): add adapters and foundations #6165

merged 13 commits into from
Jul 8, 2020

Conversation

gschrag
Copy link
Collaborator

@gschrag gschrag commented Jun 29, 2020

add foundation and adapter classes for MDCSegmentedButton and MDCSegmentedButtonSegment

abhiomkar
abhiomkar reviewed Jun 30, 2020
View reviewed changes
Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

LGTM. Nice!

Added few minor comments. Remove mdc- prefix from in PR title. (feat(segmented-button)). Send PR for review?

packages/mdc-segmented-button/segmented-button/foundation.ts Outdated Show resolved Hide resolved
packages/mdc-segmented-button/types.ts Outdated Show resolved Hide resolved
packages/mdc-segmented-button/types.ts Show resolved Hide resolved
@gschrag gschrag changed the title feat(mdc-segmented-button): add adapters and foundations feat(segmented-button): add adapters and foundations Jun 30, 2020
@kseamon
Copy link

kseamon commented Jun 30, 2020

LGTM!

@gschrag gschrag marked this pull request as ready for review July 2, 2020 00:24
@gschrag gschrag requested a review from abhiomkar July 6, 2020 20:20
abhiomkar
abhiomkar approved these changes Jul 6, 2020
View reviewed changes
Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

Left few minor comments.

packages/mdc-segmented-button/segment/adapter.ts Outdated Show resolved Hide resolved
packages/mdc-segmented-button/segment/foundation.ts Outdated Show resolved Hide resolved
packages/mdc-segmented-button/segment/foundation.ts Outdated Show resolved Hide resolved
packages/mdc-segmented-button/segmented-button/adapter.ts Outdated Show resolved Hide resolved
packages/mdc-segmented-button/segmented-button/foundation.ts Outdated Show resolved Hide resolved
@gschrag
Copy link
Collaborator Author

gschrag commented Jul 7, 2020

The build fails because there are method arguments that aren't being used until the business logic is implemented.

abhiomkar
abhiomkar reviewed Jul 7, 2020
View reviewed changes
Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

You can prefix unused variable with underscore to avoid unused lint errors. Also remove any unused empty private methods. You can add it in later PRs.

Let me know if that works.

@gschrag
Copy link
Collaborator Author

gschrag commented Jul 7, 2020

That fixed it. Now it's only failing because master's bundler has a reference to mdc-tooltip which doesn't seem to exist.

@abhiomkar
Copy link
Collaborator

Rebasing your branch with upstream should fix the issue. tooltip is newly added package.

@gschrag
Copy link
Collaborator Author

gschrag commented Jul 7, 2020 •
edited
Loading

I'm up to date with upstream master. It looks like the material-components-web build is broken too, caused by commit b0c456d. This commit reverted the commit that added mdc-tooltip by deleting its directory, but there are still references to it in https://github.com/material-components/material-components-web/blob/master/scripts/webpack/css-bundle-factory.js, and https://github.com/material-components/material-components-web/blob/master/scripts/webpack/js-bundle-factory.js.

@abhiomkar
Copy link
Collaborator

b9394dc should fix the build. can you rebase your branch?

gschrag added 13 commits July 8, 2020 10:39
@gschrag
feat: add adapters and foundations
037d4bb 
add foundation and adapter classes for MDCSegmentedButton and
MDCSegmentedButtonSegment

Links to issue #6069
@gschrag
style: add newlines to a couple files
fc0e31d 
add newlines for minor style improvement
@gschrag
style: remove whitespace and resolved comment
4b4ee33 
remove unnecessary whitespace and resolved comment
@gschrag
feat: add functionality to segment/foundation
2c271c5 
implement segment/foundation without accessibility
@gschrag
feat: add accessibility to segment
41c03dc 
add aria-checked and aria-pressed manipulation for accessibility
@gschrag
feat: add functionality to segmented-button foundation
aaccdfc 
implemented segmented-button foundation
@gschrag
chore: fix method name and file reference
50b9427 
improve name of isSingleSelect and use correct reference for @material/base
@gschrag
doc: add TSDocs to types.ts
995eea5 
add TSDoc comments to event and detail interfaces
@gschrag
refactor: change modularity of aria manipulation
df4e36d 
pull aria manipulation into helper function to clean and simplify code
@gschrag
chore: cleanup unnecessary comments and unused code
9ea4ff2 
remove TODOs and unused adapter methods
@gschrag
revert: remove business logic
a3044fc 
revert commits db4503b through
46ff243
@gschrag
chore: add TSdocs and modify methods
ec02399 
add documentation to types interfaces, remove unnused adapter methods,
add new adapter and foundation methods
@gschrag
chore: remove private method and add underscores
e162da1 
make small changes to fix build
@gschrag
Copy link
Collaborator Author

gschrag commented Jul 8, 2020

Great! This PR is ready to merge.

@abhiomkar abhiomkar merged commit 6ed717d into material-components:master Jul 8, 2020
@gschrag gschrag mentioned this pull request Jul 14, 2020
Closed
12 tasks
@adrianschmidt adrianschmidt mentioned this pull request Dec 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kseamon kseamon kseamon left review comments

@abhiomkar abhiomkar abhiomkar approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gschrag @kseamon @abhiomkar @googlebot