-
Notifications
You must be signed in to change notification settings - Fork 167
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
Shared links frontend #2890
base: master
Are you sure you want to change the base?
Shared links frontend #2890
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rerun yarn format
and also fix all the remaining issues.
src/routes/routerConfig.tsx
Outdated
{ | ||
path: 'playground/share/:uuid?', | ||
lazy: Playground | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the new share link generation will be for Source Academy @ NUS only? Why is it instead only in the public deployment route (and not for SA @ NUS)? @chownces can you confirm? I may not have the latest information from the meetings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Let's shift this to a 'with-backend' only router.
…demy/frontend into shared-links-frontend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run your lint checks locally and format the files before pushing.
I've reverted all the changes to the lockfile and TypeScript language config, as well as fixed the format errors but there are still warnings and unfixable errors. Please fix all of them, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for working on this. However, after doing a quick run-through, as it stands, this can't be merged due to numerous bugs and just code quality issues overall.
Could you improve them? Thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on refactoring the code! Can I check is this PR ready? If it's not, please mark it as draft again, thanks!
This PR has a failing test suite now:
|
src/routes/routerConfig.tsx
Outdated
{ | ||
path: 'playground/share/:uuid?', | ||
lazy: Playground | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Let's shift this to a 'with-backend' only router.
See #2937 |
* Refactor ControlBarShareButton to functional react * Update hotkeys implementation * Re-add playground configuration encoder * Migrate external URL shortener request out of sagas * Implement retrieval of playground configuration from backend UUID and reinstate configuration by hash parameters * Update test suite * Mock BrowserFS in nodejs test environment * Fix mock file error * Update hotkeys binding * Update usage of uuid decoder * Update tests
…nto shared-links-frontend
…nto shared-links-frontend
…nto shared-links-frontend
…nto shared-links-frontend
Description
Type of change
How to test
Checklist
Update on 4.12
Things still need to improve:
Personal thoughts to oop: