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(bildungsraum-share): copy content to clipboard #4354

Merged
merged 21 commits into from
Dec 19, 2024

Conversation

hejtful
Copy link
Contributor

@hejtful hejtful commented Dec 15, 2024

Linear issue: DEV-303

To test: Open any Article, Exercise or ExerciseGroup (for inspiration), choose the share option, and then click the copy content button from the modal. Paste inside a Text plugin.

@elbotho Design and text content is improvised by me, please feel free to suggest improvements.

Copy link

vercel bot commented Dec 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
frontend ✅ Ready (Inspect) Visit Preview Dec 19, 2024 2:41pm

Copy link
Contributor

github-actions bot commented Dec 15, 2024

📦 Next.js Bundle Analysis for @serlo/frontend

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 104.87 KB (🟡 +5 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Eighteen Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/[...slug] 86.71 KB (🟡 +31 B) 191.58 KB
/___editor_preview 643.13 KB (🟡 +5.91 KB) 748 KB
/content-only/[...slug] 78.81 KB (🟡 +31 B) 183.68 KB
/editor 79.62 KB (🟡 +132 B) 184.49 KB
/entity/create/[type]/[taxonomyId] 663.41 KB (🟡 +5.91 KB) 768.28 KB
/entity/repository/add-revision/[...id] 663.15 KB (🟡 +5.91 KB) 768.02 KB
/entity/repository/compare/[entity_id]/[revision_id] 91.37 KB (🟡 +33 B) 196.24 KB
/event/history 123.32 KB (🟡 +6 B) 228.19 KB
/event/history/[...slug] 124.3 KB (🟡 +6 B) 229.17 KB
/event/history/user/profile/[username] 126.69 KB (🟡 +6 B) 231.56 KB
/jobs/[[...jobId]] 51.71 KB (🟡 +30 B) 156.58 KB
/page/create 663.13 KB (🟡 +5.91 KB) 768 KB
/taxonomy/term/[copyOrMove]/batch/[id] 84.2 KB (🟡 +116 B) 189.07 KB
/taxonomy/term/create/[parent_id]/[id] 662.88 KB (🟡 +5.91 KB) 767.75 KB
/taxonomy/term/update/[id] 662.83 KB (🟡 +5.91 KB) 767.7 KB
/user/notifications 125.65 KB (🟡 +6 B) 230.52 KB
/user/profile/[username] 174.45 KB (🟡 +22 B) 279.32 KB
/user/settings 664.8 KB (🟡 +5.91 KB) 769.67 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@elbotho
Copy link
Member

elbotho commented Dec 16, 2024

When I test it in the preview I get {"kind":"not-found"}.

Might be a problem with the preview. But either wayBut I still get the 👌 Content copied! so that's unexpected.

@elbotho
Copy link
Member

elbotho commented Dec 16, 2024

On localhost it works in general.
When trying to paste the content from /1555 I get an error. Not sure why.

@elbotho
Copy link
Member

elbotho commented Dec 16, 2024

Maybe we should make the decoder a bit more relaxed and just skip unsupported plugins instead of failing completely? Not sure.

@hejtful
Copy link
Contributor Author

hejtful commented Dec 16, 2024

@elbotho Thanks for the review and testing thoroughly!

When I test it in the preview I get {"kind":"not-found"}.

Might be a problem with the preview. But either wayBut I still get the 👌 Content copied! so that's unexpected.

Tried fixing by changing the API base to staging, didn't work. I'd like to check if it's a preview issue by merging to staging, once everything else has been resolved, if you agree.

On localhost it works in general.
When trying to paste the content from /1555 I get an error. Not sure why.

Nice find! Rows plugin was missing from validation. I added it and improved logging for the future. commit

Maybe we should make the decoder a bit more relaxed and just skip unsupported plugins instead of failing completely? Not sure.

Not sure either, I'm afraid that would lead to frustration, if the user expects everything to be copied, and it's not. We could show a snack saying that the plugin of a certain type was not copied, because it's invalid, but not sure how helpful that would be, it will still be frustrating to look through a whole article of rows to see what wasn't copied.

@elbotho
Copy link
Member

elbotho commented Dec 17, 2024

Not sure either, I'm afraid that would lead to frustration, if the user expects everything to be copied, and it's not. We could show a snack saying that the plugin of a certain type was not copied, because it's invalid, but not sure how helpful that would be, it will still be frustrating to look through a whole article of rows to see what wasn't copied.

I suspect the frustration is even bigger if you want to import an article and it does not work at all because of one plugin.
If you don't mind and if it's not too hard (nesting…) I'd prefer to import everything but the failing plugins. Maybe with an "unsupported" plugin their place.

@hejtful
Copy link
Contributor Author

hejtful commented Dec 17, 2024

I suspect the frustration is even bigger if you want to import an article and it does not work at all because of one plugin.

I wouldn't agree, but respect your UX seniority 😁

too hard (nesting…)

The state decoder I borrowed from paste-hack didn't validate nested plugins anyway.

Maybe with an "unsupported" plugin their place.

The default functionality of the editor is to replace invalid plugins with the Unsupported plugin.

I'd prefer to import everything but the failing plugins.

So, do you prefer:

  1. Being very helpful and validating nested plugins, and showing a snack for each invalid plugin type in the state? (complex TS magic for nested decoder)
  2. Relying on default editor handling of invalid plugins? (very fast, no TS magic)

I already have the in-between solution of validating first level plugins and showing snacks for them, but a half-solution like that isn't great.

@elbotho
Copy link
Member

elbotho commented Dec 17, 2024

Relying on default editor handling of invalid plugins? (very fast, no TS magic)

def. this.
Maybe adding a single toast like: "there were upsupported plugins…".

@hejtful
Copy link
Contributor Author

hejtful commented Dec 18, 2024

@elbotho Ok, current state:

  • Only validate basic Rows plugin structure. I'm thinking it's still good to fail if the root Rows plugin is invalid.
  • In order to only notify of unsupported plugins on paste, added a custom native event helper. Cleanest solution I could think of + can be re-used if a need, to notify of unsupported plugins in other places, arises.

Would you mind adding the missing German strings that break the pipeline (or just writing them here and I'll add them)?

Copy link
Member

@elbotho elbotho left a comment

Choose a reason for hiding this comment

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

Thank you for the improvements!
After this PR is merged I think we can test this in staging.

feat(copy content button): make "de" only and add some explanation
@hejtful hejtful merged commit 33ecaad into staging Dec 19, 2024
9 checks passed
@hejtful hejtful deleted the feat/bildungsraum-share-copy-editor-content branch December 19, 2024 14:44
@github-actions github-actions bot mentioned this pull request Dec 19, 2024
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