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: change file for exists share #306

Merged

Conversation

IvanLi-CN
Copy link
Contributor

@IvanLi-CN IvanLi-CN commented Nov 1, 2023

close #154 .

This is the effect achieved. Is there anything that needs to be adjusted in the process?

output_hevc.mp4

@stonith404
Copy link
Owner

Thanks! I'll look into it at the weekend.

Copy link
Owner

@stonith404 stonith404 left a comment

Choose a reason for hiding this comment

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

Sorry for being so picky, but these should be only small changes. I really like the implementation of the feature. Thank you! :)

@IvanLi-CN
Copy link
Contributor Author

Thank you for your suggestions. The points you mentioned are also areas where I am hesitant. I will revise them before submitting.

In addition, I would like to know, from your perspective, after the user modifies the Share and clicks the save button, do you directly prompt the user with a toast or pop up the current modal box? And should I stay on the current page instead of jumping to the home page?

@stonith404

@stonith404
Copy link
Owner

@IvanLi-CN, for me, it doesn't really matter. Do what you think is best. If you ask for my opinion, I would suggest redirecting back to the "My Shares" page, as that is the page the user is coming from.

@IvanLi-CN IvanLi-CN force-pushed the feat/add-remove-file-for-exists-share branch from fd04aff to a7c4a09 Compare November 4, 2023 02:38
@IvanLi-CN
Copy link
Contributor Author

Modified, please check 😋 @stonith404

Copy link
Owner

@stonith404 stonith404 left a comment

Choose a reason for hiding this comment

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

Great, thank you! Can I merge?

@IvanLi-CN
Copy link
Contributor Author

Great, thank you! Can I merge?

Do it! 😋

@stonith404 stonith404 merged commit 98380e2 into stonith404:main Nov 4, 2023
1 check passed
@pierrbt
Copy link
Contributor

pierrbt commented Nov 4, 2023

Wow nice changes, good job !

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.

🚀 Feature: Update files in an existing share
3 participants