Skip to content

Conversation

@KkkkYL
Copy link

@KkkkYL KkkkYL commented Aug 25, 2023

Resolve that box shadow cannot be displayed without inset attribute and has no blur effect when blur radius is set.

version 1.4.1

(I must say that this is not a perfect effect, and there is still a slight difference between the shadow style and the actual style.)

berfore:

old hidden old visible

now:

new hidden new visible

package.json Outdated
"license": "MIT",
"dependencies": {
"css-line-break": "^2.1.0",
"less": "^4.2.0",

Choose a reason for hiding this comment

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

Ouch. Adding less to the project is already not a good idea, but adding it to the dependencies...

Can you provide your PR without less?

initialValue: '0% 0%',
type: PropertyDescriptorParsingType.LIST,
prefix: false,
parse: (_context: Context, tokens: CSSValue[]): BackgroundPosition => {

Choose a reason for hiding this comment

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

This change introduces no improvement and is actually more confusing due to unnamed variables.

prefix: false,
type: PropertyDescriptorParsingType.LIST,
parse: (_context: Context, tokens: CSSValue[]): BackgroundSize => {
// console.log('bakground-size')

Choose a reason for hiding this comment

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

Remove dead code

}

return parseFunctionArgs(tokens).map((values: CSSValue[]) => {
let c = parseFunctionArgs(tokens).map((values: CSSValue[]) => {

Choose a reason for hiding this comment

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

This change introduces no improvement and is actually more confusing due to unnamed variables.

if (this.styles.transform !== null) {
// getBoundingClientRect takes transforms into account
element.style.transform = 'none';
console.log('isHTMLElementNode')

Choose a reason for hiding this comment

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

Can you remove ?

this.ctx.save();
const borderBoxArea = calculateBorderBoxPath(paint.curves);
const maskOffset = shadow.inset ? 0 : MASK_OFFSET;
const maskOffset = shadow.inset ? 0 : 1;

Choose a reason for hiding this comment

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

This might be correct, but I find it suspicious that switching a value from 10000 to 1 is not having bad impacts somewhere. Are you sure that this cannot introduce bugs elsewhere?

add(deltaX: number, deltaY: number): Vector {
return new Vector(this.x + deltaX, this.y + deltaY);
}
reverse(): Vector {

Choose a reason for hiding this comment

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

Is it really necessary to create a function that just returns this?

Copy link
Author

Choose a reason for hiding this comment

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

If this is not done, the border radius in canvas will not display correctly.

@Sharcoux
Copy link

Hey, I'm trying to get promising PRs to be merged. Can you check my review? Some parts are uncleared to me but I have meny PRs to review so I didn't dig too deep into it. Don't hesitate to comment.

@Sharcoux
Copy link

Can be tested in @cantoo/html2canvas

@KkkkYL
Copy link
Author

KkkkYL commented Sep 5, 2023

Okay, I will modify the unreasonable code.

@KkkkYL
Copy link
Author

KkkkYL commented Sep 5, 2023

@Sharcoux Is it okay now?

@Sharcoux
Copy link

Sharcoux commented Sep 5, 2023

Actually I can't merge in this repo, but I released @cantoo/html2canvas that includes your change. You can test it and use it if you need.

@KkkkYL
Copy link
Author

KkkkYL commented Sep 5, 2023

ok!

nangelina added a commit to artificialonlinesao/html2canvas that referenced this pull request Feb 9, 2024
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.

2 participants