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

feat: add gradient fill functionality for QR codes #87

Merged
merged 3 commits into from
Nov 2, 2024

Conversation

WuChenDi
Copy link
Contributor

  • Introduced gradient-related properties in the QrcodeVue component to allow for customizable QR code appearances.
  • Implemented both linear and radial gradient options for enhanced visual appeal.
  • Updated the README documentation with examples and instructions for using the gradient features.
  • Modified the example project to include controls related to gradient options for user testing and demonstration.

- Introduced gradient-related properties in the QrcodeVue component to allow for customizable QR code appearances.
- Implemented both linear and radial gradient options for enhanced visual appeal.
- Updated the README documentation with examples and instructions for using the gradient features.
- Modified the example project to include controls related to gradient options for user testing and demonstration.
@WuChenDi
Copy link
Contributor Author

First of all, thanks to @kerimovok for their contribution!

These features were developed based on their ideas—I simply organized and submitted the PR for merging into the main branch. For more details, please refer to #78.

@scopewu scopewu self-requested a review October 31, 2024 06:26
@scopewu
Copy link
Owner

scopewu commented Oct 31, 2024

Hi,
Some code changes emit an error Uncaught (in promise) Maximum recursive updates exceeded. imageProps.value is an object type that triggers the onUpdated cycle every time it is changed.

-   let imageProps: { x: number, y: number, width: number, height: number }
+   const imageProps = ref<{ x: number, y: number, width: number, height: number } | null>(null)

- Removed the ref wrapping of imageProps, converting it to a plain object for simpler management.
- Simplified the assignment logic of imageProps for better readability and maintainability.
- Eliminated unnecessary handling of null values to streamline the code.
@WuChenDi
Copy link
Contributor Author

Hi, Some code changes emit an error Uncaught (in promise) Maximum recursive updates exceeded. imageProps.value is an object type that triggers the onUpdated cycle every time it is changed.

-   let imageProps: { x: number, y: number, width: number, height: number }
+   const imageProps = ref<{ x: number, y: number, width: number, height: number } | null>(null)

done

@scopewu
Copy link
Owner

scopewu commented Oct 31, 2024

Hi, We need to check the code and make minimal changes, deleting old fields when changing here.

<!-- webpack.html -->
<div class="row mb-3">
  <label class="col-sm-2 col-form-label">Foreground:</label>
  <div class="col-sm-10">
    <input type="color" class="border border-3 rounded" v-model="foreground">
  </div>
</div>

Sometimes we need to use named parameter arguments so that it is easy to maintain. Also it seems that there are too many parameters, and we need to support the functionality proposed in issue #80 later. There will be too many parameters, so we may need to refactor the documentation.

有时我们需要使用具名参数参数,这样好维护。另外似乎参数有点过多,后面还要支持 issue #80 提出的功能。参数会过多,可能另外需要重构文档。

image

@WuChenDi
Copy link
Contributor Author

This part can be considered for optimization in the next version. Listing all the props individually feels cumbersome, so I simplified it by passing the entire props object directly.

Or do you think it’s necessary to define each one individually?

这个部分可以在下一个版本中再考虑优化。目前列出所有 props 显得有些繁琐,所以我进行了简化,直接传入整个 props 对象即可。

或者你认为有必要一个个列举出来定义吗?

@scopewu
Copy link
Owner

scopewu commented Oct 31, 2024

有历史原因,有过滤参数的目的。

@scopewu
Copy link
Owner

scopewu commented Oct 31, 2024

If continue to add multiple features, it may cause problems, and performance also needs to be considered.

如果继续添加功能,可能会出现问题,同时性能也需要考虑。

@scopewu
Copy link
Owner

scopewu commented Oct 31, 2024

I'll continue later tonight.

我今晚继续,现在有事。

@WuChenDi
Copy link
Contributor Author

Oh, I’m very sorry.

This is my first time contributing a PR to this project, and I wasn’t aware of the historical context here. I have now reverted this part of the logic to keep it consistent with the previous version.

@scopewu scopewu mentioned this pull request Oct 31, 2024
@scopewu
Copy link
Owner

scopewu commented Oct 31, 2024

This thing is a joy, only in code, nothing else, thanks for putting in the effort. Will merge to master branch if there are no major issues. Thanks again.

Copy link
Owner

@scopewu scopewu left a comment

Choose a reason for hiding this comment

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

The codes is okay with checked. Will be merged.

@scopewu scopewu self-assigned this Nov 2, 2024
@scopewu scopewu merged commit f194229 into scopewu:main Nov 2, 2024
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.

2 participants