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

fix(progress-bar): remove data url from progress-bar's css styling #8898

Merged
merged 1 commit into from
Jan 26, 2018
Merged

fix(progress-bar): remove data url from progress-bar's css styling #8898

merged 1 commit into from
Jan 26, 2018

Conversation

josephperrott
Copy link
Member

Fixes #8876

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 8, 2017
crisbeto
crisbeto reviewed Dec 8, 2017
View reviewed changes
src/lib/progress-bar/progress-bar.html Outdated
<div class="mat-progress-bar-background mat-progress-bar-element">
<svg width="100%" height="5">
<defs>
<pattern id="circle" x="5" y="0" width="10" height="5" patternUnits="userSpaceOnUse">
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK this ID has to be unique.

src/lib/progress-bar/progress-bar.ts Outdated
* then placed back into the DOM again. This is necessary as the browser only defines the
* styling of SVG pattern elements as it is placed into the DOM.
*/
private _reappendCircleSvgElement() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced that this is necessary. The progress spinner supports changing the colors without having to re-append the element to the DOM. Are you sure that it doesn't have something to do with the circles not having a unique id?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are right

@josephperrott josephperrott force-pushed the progres-bar branch 3 times, most recently from e6cc39d to 97f8482 Compare December 8, 2017 22:17
jelbourn
jelbourn reviewed Dec 13, 2017
View reviewed changes
src/lib/progress-bar/progress-bar.html Outdated
<div class="mat-progress-bar-background mat-progress-bar-element">
<svg width="100%" height="5">
<defs>
<pattern id="{{progressbarId}}" x="5" y="0" width="10" height="5" patternUnits="userSpaceOnUse">
Copy link
Member

Choose a reason for hiding this comment

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

Use [id] instead of interpolation?

src/lib/progress-bar/progress-bar.scss Outdated
background-size: 10px 4px;
display: none;
svg {
position: absolute;
Copy link
Member

Choose a reason for hiding this comment

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

Also comment on why the element is absolute?

@josephperrott josephperrott force-pushed the progres-bar branch 2 times, most recently from 8ec3e4b to 237b400 Compare December 15, 2017 19:23
@josephperrott josephperrott force-pushed the progres-bar branch 2 times, most recently from a3a7515 to 32ae3b0 Compare January 25, 2018 16:23
@josephperrott
Copy link
Member Author

@jelbourn ready for another look when you have a moment.

jelbourn
jelbourn approved these changes Jan 25, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 25, 2018
@jelbourn jelbourn merged commit 0f2ac9b into angular:master Jan 26, 2018
@jelbourn jelbourn added the target: minor This PR is targeted for the next minor release label Jan 29, 2018
@crisbeto crisbeto mentioned this pull request Aug 17, 2018
Merged
@angular-automatic-lock-bot
Copy link

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-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
@josephperrott josephperrott deleted the progres-bar branch March 20, 2020 22:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Buffer progress bar "ball" is blocked by CSP due to data url svg
4 participants
@josephperrott @jelbourn @crisbeto @googlebot