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

Revisions number limit #391

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

SiriusXT
Copy link

@SiriusXT SiriusXT commented Sep 4, 2024

Related to #262
图片

Copy link

@JYC333 JYC333 left a comment

Choose a reason for hiding this comment

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

Thanks for this great work! I have one comment here.

image

I think it's good to add some notifications/information in this Note revisions modal to let user know what's the limit they have now. It's in src/public/app/widgets/dialogs/revisions.js. Not sure where could be the best place to add, maybe just right after the Note revisions title?

@SiriusXT
Copy link
Author

How does this layout feel
图片

@JYC333
Copy link

JYC333 commented Sep 13, 2024

Looks good! Just remind that, please don't put -1 for unlimit here, use something else :) Maybe an infinite symbol could be interesting, but just my thought, depends on how hard to implement. Using a word is also good enough!

@eliandoran
Copy link

How does this layout feel 图片

Personally I wouldn't put it in the title bar since it complicates a bit the layout.

My recommendation would be to put it at the end of the dialog, either right underneath the list of revisions or at the bottom as a kind of a status bar.

Regardless, it shouldn't be as big as the title, as the size hierarchy is important and the limit of revisions is not as important as the title of the modal, so I would recommend making it default text size.

I would also rephrase the text slightly to something like "Maximum revisions per note: ". Love the Gear button, tho.

@SiriusXT
Copy link
Author

Now:
图片

@SiriusXT SiriusXT marked this pull request as draft September 14, 2024 12:00
@eliandoran
Copy link

@SiriusXT , tested locally and works really good. I see you put it again in draft?

@SiriusXT
Copy link
Author

Because a bug was found that for svg revisions, there were some problems with previewing in revisions, the last two commits fixed the problem.

There is also a bug that is not related to this pr, for image/svg notes, by overwriting the image with another format, the original image becomes the revisions, but cannot be restored. May need to limit uploading images in different formats or find another way.

@SiriusXT SiriusXT marked this pull request as ready for review September 14, 2024 13:15
@eliandoran
Copy link

Because a bug was found that for svg revisions, there were some problems with previewing in revisions, the last two commits fixed the problem.

Was this bug introduced by this PR or was it already present?

There is also a bug that is not related to this pr, for image/svg notes, by overwriting the image with another format, the original image becomes the revisions, but cannot be restored. May need to limit uploading images in different formats or find another way.

Interesting, please don't forget to create an issue for it in order to track it.

@SiriusXT
Copy link
Author

This bug has existed before, including in the zadam/trilium repository.

I will track the bug about different image format revisions in a separate issue later.

@SiriusXT
Copy link
Author

SiriusXT commented Sep 15, 2024

There is also a bug that is not related to this pr, for image/svg notes, by overwriting the image with another format, the original image becomes the revisions, but cannot be restored. May need to limit uploading images in different formats or find another way.
This bug has existed before, including in the zadam/trilium repository.

The bug has now been fixed by updating the note's type and MIME to match the revision's type and MIME when restoring the note.

@JYC333
Copy link

JYC333 commented Sep 15, 2024

I found the ESC behaviour is bit wierd so I just make changes from my side, hope you don't mind.

Since we are using dropdown for revision list, then pressing ESC now will close the dropdown. I guess it's some side effect from upgrading Bootstrap. Now it should be fixed, and pressing ESC when the revision element is focused will close the dialog also.

I'm good with your other changes and great work!

@SiriusXT
Copy link
Author

SiriusXT commented Sep 15, 2024

I found the ESC behaviour is bit wierd so I just make changes from my side, hope you don't mind.

Since we are using dropdown for revision list, then pressing ESC now will close the dropdown. I guess it's some side effect from upgrading Bootstrap. Now it should be fixed, and pressing ESC when the revision element is focused will close the dialog also.

I'm good with your other changes and great work!

Great. I also encountered the revision lists left column suddenly disappearing before. I always thought there was something wrong with its layout.

@JYC333
Copy link

JYC333 commented Sep 15, 2024

I found this when I upgraded Bootstrap. Using dropdown probably not the best choice, but I think I will leave it for now since it's working fine :)

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.

3 participants