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

Shared links frontend #2890

Draft
wants to merge 37 commits into
base: master
Choose a base branch
from
Draft

Shared links frontend #2890

wants to merge 37 commits into from

Conversation

Rachelcoll
Copy link

@Rachelcoll Rachelcoll commented Mar 30, 2024

Description

  1. change of shared links routes (/plaground/share/:uuid)'
  2. change shared links generation procedure (check ControllBarShareButton.tsx and Encoder.tsx)
  3. change procedure of accepting and identify shared links (check Playground.tsx and Decoder.tsx)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

Checklist

  • I have tested this code (all functionality works on my local computer fine, not sure why it cannot pass pre-push hooks check)
  • I have updated the documentation

Update on 4.12
Things still need to improve:

  1. Cannot find Wrapper function for fetch request
  2. How to fetch frontend url since there's no Constant frontend url
  3. Still fail to pass 4 Playground test

Personal thoughts to oop:

  1. Personally I suggest remove OOP design of Encoder since currently we only need new way to generate url, no need to generate queryString

Copy link
Member

@RichDom2185 RichDom2185 left a 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.

package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
Comment on lines 71 to 74
{
path: 'playground/share/:uuid?',
lazy: Playground
},
Copy link
Member

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.

Copy link
Contributor

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.

src/pages/playground/Encoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Encoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Decoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
@RichDom2185
Copy link
Member

image

Please run your lint checks locally and format the files before pushing.

Copy link
Member

@RichDom2185 RichDom2185 left a 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!

Copy link
Member

@RichDom2185 RichDom2185 left a 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!

package.json Outdated Show resolved Hide resolved
src/commons/controlBar/ControlBarShareButton.tsx Outdated Show resolved Hide resolved
src/commons/controlBar/ControlBarShareButton.tsx Outdated Show resolved Hide resolved
src/commons/controlBar/ControlBarShareButton.tsx Outdated Show resolved Hide resolved
src/commons/controlBar/ControlBarShareButton.tsx Outdated Show resolved Hide resolved
src/pages/playground/Encoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Encoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Encoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Encoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Encoder.tsx Outdated Show resolved Hide resolved
Copy link
Member

@RichDom2185 RichDom2185 left a 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!

yarn.lock Outdated Show resolved Hide resolved
src/features/playground/shareLinks/ShareLinkState.tsx Outdated Show resolved Hide resolved
src/features/playground/shareLinks/ShareLinkState.tsx Outdated Show resolved Hide resolved
src/features/playground/shareLinks/encoder/URLEncoder.tsx Outdated Show resolved Hide resolved
src/features/playground/shareLinks/encoder/URLEncoder.tsx Outdated Show resolved Hide resolved
src/features/playground/shareLinks/encoder/URLEncoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
@martin-henz
Copy link
Member

This PR has a failing test suite now:

FAIL src/pages/playground/__tests__/Playground.tsx
  ● Test suite failed to run

src/commons/controlBar/ControlBarShareButton.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
@Rachelcoll
Copy link
Author

Basically there's not much UI added in this PR, so I just put a demo video of whole process here.

51b071877c9363ff2759c02e7b3f82bc_raw.mp4

Error page when fail to grab program from share link (due to invalid url or web disconnected:
b8fc576e7a2202a3296d81f90d73e24

Error page when fail to create share link:
9fe886cf5127dccb75bf07551f81d3b

src/features/playground/shareLinks/ShareLinkState.ts Outdated Show resolved Hide resolved
src/commons/controlBar/ControlBarShareButton.tsx Outdated Show resolved Hide resolved
src/commons/controlBar/ControlBarShareButton.tsx Outdated Show resolved Hide resolved
src/features/playground/shareLinks/encoder/Encoder.tsx Outdated Show resolved Hide resolved
src/pages/playground/Playground.tsx Outdated Show resolved Hide resolved
Comment on lines 71 to 74
{
path: 'playground/share/:uuid?',
lazy: Playground
},
Copy link
Contributor

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.

@chownces
Copy link
Contributor

See #2937

@Rachelcoll Rachelcoll marked this pull request as draft April 15, 2024 06:46
chownces and others added 2 commits April 15, 2024 17:02
* 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
@coveralls
Copy link

coveralls commented Apr 15, 2024

Pull Request Test Coverage Report for Build 9107001964

Details

  • 79 of 127 (62.2%) changed or added relevant lines in 12 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.2%) to 31.601%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/features/playground/shareLinks/decoder/delegates/JsonDecoderDelegate.ts 0 1 0.0%
src/features/playground/shareLinks/encoder/delegates/JsonEncoderDelegate.ts 0 1 0.0%
src/features/playground/shareLinks/encoder/delegates/UrlParamsEncoderDelegate.ts 0 1 0.0%
src/features/playground/shareLinks/encoder/Encoder.ts 1 3 33.33%
src/pages/playground/Playground.tsx 22 25 88.0%
src/features/playground/shareLinks/encoder/EncoderHooks.ts 2 21 9.52%
src/commons/controlBar/ControlBarShareButton.tsx 18 39 46.15%
Files with Coverage Reduction New Missed Lines %
src/features/playground/PlaygroundReducer.ts 1 60.0%
src/pages/playground/Playground.tsx 1 56.17%
Totals Coverage Status
Change from base Build 9101202777: 0.2%
Covered Lines: 4924
Relevant Lines: 14697

💛 - Coveralls

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.

None yet

5 participants