Skip to content

feat(progress-spinner): create progress-spinner based on MDC Web - #20048

Merged
jelbourn merged 18 commits into
angular:masterfrom
annieyw:mdc-progress-spinner
Jul 31, 2020
Merged

feat(progress-spinner): create progress-spinner based on MDC Web#20048
jelbourn merged 18 commits into
angular:masterfrom
annieyw:mdc-progress-spinner

Conversation

@annieyw

Copy link
Copy Markdown
Contributor

No description provided.

@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 20, 2020
@annieyw
annieywforce-pushed the mdc-progress-spinner branch 3 times, most recently from ca84a45 to bcac8f0CompareJuly 21, 2020 23:22
@annieyw
annieyw marked this pull request as ready for review July 22, 2020 16:52
@annieyw
annieyw requested a review from devversion as a code ownerJuly 22, 2020 16:52
@annieyw
annieyw requested a review from a teamJuly 22, 2020 16:52
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.html Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.scss Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
@crisbeto

Copy link
Copy Markdown
Member

One more thing: it doesn't look like the server-side rendering check was set up for the new component.

Comment threadsrc/dev-app/mdc-progress-spinner/mdc-progress-spinner-demo.scss Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/_progress-spinner-theme.scss Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.scss Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/module.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.html Outdated
</div>
</div>
</div>
</div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we know why the MDC spinner is so much more complicated than our existing one? I'm surprised that this has four SVGs when the current spinner uses just one (with two circle elements)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think it may be because the MDC spinner supports 4 colors but I'm not sure

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Four colors at the same time? It would be good to just check in with them to get some background so that we know why it's built this way

Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.scss Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
@annieyw
annieywforce-pushed the mdc-progress-spinner branch from 1a7ef3e to 69605dcCompareJuly 24, 2020 19:30
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.html Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.scss Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.html Outdated
</div>
</div>
</div>
</div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Four colors at the same time? It would be good to just check in with them to get some background so that we know why it's built this way

Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.scss Outdated
Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
@annieyw
annieywforce-pushed the mdc-progress-spinner branch from 5f6798b to d9592f5CompareJuly 27, 2020 21:47
@annieyw
annieywforce-pushed the mdc-progress-spinner branch from d9592f5 to de98adfCompareJuly 27, 2020 21:50

@mmalerbammalerba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment threadsrc/material-experimental/mdc-progress-spinner/progress-spinner.ts Outdated
@mmalerbammalerba added lgtm target: patch This PR is targeted for the next patch release labels Jul 28, 2020
@annieywannieyw added the action: merge The PR is ready for merge by the caretaker label Jul 28, 2020

@jelbournjelbourn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM- we can figure out more about the multiple svgs as we go

Comment threadsrc/material-experimental/mdc-theming/_all-theme.scss Outdated
@mmalerbammalerba removed the lgtm label Jul 31, 2020
@jelbourn
jelbourn merged commit 294b8ee into angular:masterJul 31, 2020
jelbourn pushed a commit that referenced this pull request Jul 31, 2020
…on MDC Web (#20048)
* add files for MDC-based MatProgressSpinner
* add demo of mdc-based MatProgressSpinner at localhost:4200/mdc-progress-spinner
* copy MDC DOM and add MDC styles
* set up MDC foundation for MDC-based MatProgressSpinner
* (wip) add adapter and fill in API
* copy over existing demo to MDC-based demo
* serverside rendering, refactor
* add tests (adapter class wip)
* remove separate class for MatSpinner
* add e2e tests
* add harness
* add user documantation README
* add codeowner, theme dep
* address CI failures
* export MatSpinner as const to avoid build error
* fix missing e2e import
* fix nits
* remove changes to _all-themes to keep PR merge-safe (add back in followup)
(cherry picked from commit 294b8ee)
@annieyw
annieyw deleted the mdc-progress-spinner branch August 3, 2020 16:51
@mmalerbammalerba changed the title prototype(progress-spinner): create prototype progress-spinner based on MDC Webfeat(progress-spinner): create prototype progress-spinner based on MDC WebAug 26, 2020
@mmalerbammalerba changed the title feat(progress-spinner): create prototype progress-spinner based on MDC Webfeat(progress-spinner): create progress-spinner based on MDC WebAug 26, 2020
@angular-automatic-lock-bot

Copy link
Copy Markdown

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-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 26, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License Agreementtarget: patchThis PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@annieyw@crisbeto@jelbourn@mmalerba@googlebot