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

✨ [progress] add progress #10

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Muzych
Copy link
Contributor

@Muzych Muzych commented Mar 15, 2024

πŸ€” Nature of this PR

  • πŸ› bug fix
  • ✨ new feature
  • πŸ“ documentation improvement
  • πŸ› demo code improvement
  • πŸš€ component style/interaction improvement
  • πŸ—οΈ ci/cd improvement
  • ♻️ refactoring
  • 🎨 code style optimization
  • βœ… test cases
  • πŸ”€ branch merging
  • πŸ’‘ other

πŸ”— Related Issue

πŸ’‘ Background and Solution

βœ… Pre-merge Checklist

❗️Please self-check and check all options.❗️

  • Documentation is supplemented or not needed
  • Code demonstration is provided or not needed
  • TypeScript definitions are supplemented or not needed
  • Changelog is provided or not needed

Copy link
Member

Choose a reason for hiding this comment

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

defineCore in vue version has some "historical reasons".
I don't think this is necessary in react version.

export const leaf = leafPng;


export function useProgress(options: Options<{
Copy link
Member

Choose a reason for hiding this comment

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

try this:

export function useProgress(props:ProgressProps) 

@@ -0,0 +1,65 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

copy file from vue version , you should update comments

useEffect(() => {
setValue(props.value ?? 0);
setMax(props.max ?? 100);
}, [props.value, props.max]);
Copy link
Member

Choose a reason for hiding this comment

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

should value and max use two useEffect?
Their changes don't seem to affect each other.

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