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

fix(ie): make IE preserve other browsers' image width styling (#1275) #1276

Merged
merged 1 commit into from Jun 17, 2019

Conversation

SpencerWoo
Copy link
Member

…e styling

#1275

Instead of removing all the styles, keep the width (controls sizing) style

@wincent wincent changed the title Fix https://github.com/liferay/alloy-editor/issues/1275 | Persist wid… fix(ie): make IE preserve other browsers' image width styling (#1275) Jun 7, 2019
image.removeAttribute('style');
let imageStyles = image.getAttribute('style');
let widthStyles = /(width:.*?;)/g.exec(imageStyles);
let widthStyle = widthStyles[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems all of these let should be const, right?

Also, about that regex: .*? is a non-greedy match (good), but * implies that zero-width is acceptable and it isn't, is it? ie. .+? would be a more accurate specification of what we're looking for. Not that I think we'll get a zero-width value there in practice, just that I think in general it's best to keep regexes as "tight" as possible.

Copy link
Member Author

@SpencerWoo SpencerWoo Jun 7, 2019

Choose a reason for hiding this comment

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

Yeah, they can all be const. Wasn't aware that's the convention but that makes sense.

Also good on .+? -- that's definitely better here

@SpencerWoo
Copy link
Member Author

SpencerWoo commented Jun 11, 2019

Didn't know if we were waiting for me to make those changes but I've updated my commit to reflect the feedback

Let me know if there's anything else or when this can be merged so we can handle the LPS. thanks

@SpencerWoo
Copy link
Member Author

@wincent Any update on reviewing this one? thanks

@wincent
Copy link
Contributor

wincent commented Jun 17, 2019

@SpencerWoo: We were looking this over but we had two days out due to office activities here in Madrid. @julien was testing it out and is going to merge it and cut a release today.

@SpencerWoo
Copy link
Member Author

awesome thanks guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants