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

Refactor toast in presentation-uploader #15474

Merged

Conversation

GuiLeme
Copy link
Collaborator

@GuiLeme GuiLeme commented Aug 2, 2022

What does this PR do?

It refactors the toast to not depend on the presentation-uploader component, instead, it will be a separate component that will watch any presentation coming from the service of presentation uploader, and so, can be used in every ocasion the client wants to send a document. Moreover, it will notify the presenter of possible conversions that might be occurring, since it is watching the Presentations collection as well.

Closes Issue(s)

#15505

More

Credits and huge thanks to @JoVictorNunes, who made a patch to insert the count of pages in the conversion part, closing issue #15505:

GuiLeme#6

@github-actions
Copy link

github-actions bot commented Aug 2, 2022

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@antobinary antobinary added this to the Release 2.6 milestone Aug 2, 2022
@antobinary antobinary marked this pull request as draft August 2, 2022 21:10
@github-actions
Copy link

github-actions bot commented Aug 5, 2022

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@GuiLeme GuiLeme marked this pull request as ready for review August 9, 2022 17:08
Comment on lines 275 to 276
// tmpIdconvertingPresentations = UploadingPresentations.find({}).fetch().filter(p => tmpIdconvertingPresentations.includes(p.tmpPresId))
// .map(p => p.tmpPresId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// tmpIdconvertingPresentations = UploadingPresentations.find({}).fetch().filter(p => tmpIdconvertingPresentations.includes(p.tmpPresId))
// .map(p => p.tmpPresId)

.map(p => UploadingPresentations.remove({tmpPresId: p.tmpPresId}));
// Remove all presentations from the uploading collection if they are already
// converting.
// UploadingPresentations.remove({tmpPresId: {$all: tmpIdconvertingPresentations}});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// UploadingPresentations.remove({tmpPresId: {$all: tmpIdconvertingPresentations}});

Comment on lines 81 to 82

// Fazer o onConversion modificar a upload e ver o que mais da pra fazer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Fazer o onConversion modificar a upload e ver o que mais da pra fazer

GuiLeme and others added 2 commits August 22, 2022 15:54
Co-authored-by: João Victor Nunes <62393923+JoVictorNunes@users.noreply.github.com>
fix: show conversion process info in toast
Copy link
Collaborator

@JoVictorNunes JoVictorNunes left a comment

Choose a reason for hiding this comment

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

The client part seems all ok! Let's hold on @gustavotrott's review on the backend part.

Copy link
Collaborator

@gustavotrott gustavotrott left a comment

Choose a reason for hiding this comment

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

image(14)

I can still reproduce the problem related in #15259.
Although this PR brings great improvements to the pres-upload toast! If you want, we can merge this changes and try to fix the problem in a subsequent PR. It seems to be related with Tldraw component only.

@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@danielpetri1
Copy link
Collaborator

Awesome stuff, after some testing it looks like #15259 and #15773 are fixed now with the latest commits! 🎉🎉 With the progress showing too!🥳

When sending a presentation to chat, however, the toast message doesn't go away once it converts. Any clues as to why that could be?

Copy link
Collaborator

@gustavotrott gustavotrott left a comment

Choose a reason for hiding this comment

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

LGTM! Really good improvements!

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