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.

Sign up for GitHub

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

Jump to bottom

Make toast support preventDuplicates #31501

Merged
merged 15 commits into from
Jun 27, 2024
Merged

Make toast support preventDuplicates #31501

merged 15 commits into from
Jun 27, 2024

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Jun 26, 2024 •
edited by silverwind
Loading

make preventDuplicates default to true, users get a clear UI feedback and know that "a new message appears".

Fixes: #26651

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 26, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2024
@wxiaoguang wxiaoguang mentioned this pull request Jun 26, 2024
Draft
@github-actions github-actions bot added modifies/templates This PR modifies the template files modifies/js labels Jun 26, 2024
@wxiaoguang wxiaoguang added the type/enhancement An improvement of existing functionality label Jun 26, 2024
@silverwind
Copy link
Member

silverwind commented Jun 26, 2024 •
edited
Loading

we should hide the existing toast and show the new toast

As I said, I think this is suboptimal UX because it makes it harder then necessary to copy text out a toast that re-creates for example every 1-2 seconds. I think it's better to have a counter instead that shows the number of times a toast has triggered. Could be as simple as appending (2) to the message. Same mechanism we alredy have in place in the global js error handler.

@wxiaoguang
Copy link
Contributor Author

Could be as simple as appending (2) to the message.

f6034b2

@silverwind
Copy link
Member

silverwind commented Jun 26, 2024 •
edited
Loading

Looks quite good but I noticed that the animation only plays when it goes from 1 to 2, but not from 2 to 3 or later, is it intentional?

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Jun 26, 2024 •
edited
Loading

Looks quite good but I noticed that the animation only plays when it goes from 1 to 2, but not from 2 to 3 or later, is it intentional?

It works on my side, and the code seems right.

@silverwind
Copy link
Member

silverwind commented Jun 26, 2024 •
edited
Loading

Subsequent animations work for me only in Chrome, not in Firefox.

@silverwind
Copy link
Member

Could you add to devtest a second row of buttons that generates different messages?

@wxiaoguang
Copy link
Contributor Author

Subsequent animations work for me only in Chrome, not in Firefox.

6a9ed8c

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 26, 2024
@yp05327
Copy link
Contributor

yp05327 commented Jun 27, 2024 •
edited
Loading

Nice fix. Can you add fix #26651 in the description.

@yp05327
Copy link
Contributor

yp05327 commented Jun 27, 2024 •
edited
Loading

Maybe we can add more spaces here? Then the animation seems better.
image

@yp05327 yp05327 added the topic/ui Change the appearance of the Gitea UI label Jun 27, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2024
@silverwind
Copy link
Member

Nice fix. Can you add fix #26651 in the description.

Added

@yp05327
Copy link
Contributor

yp05327 commented Jun 27, 2024 •
edited
Loading

before:
image
after:
image
It seems that the icon is too big?
But I can't see this in @wxiaoguang 's screenshot 🤔

@yp05327
Copy link
Contributor

yp05327 commented Jun 27, 2024 •
edited
Loading

image
image

If you scale the screen to 300%, you can see it clearly.
image

ps: same to another issue that caused by different OS and browser?

@wxiaoguang
Copy link
Contributor Author

I don't want to play with the pixel-level tricks here.

The UI doesn't really look problematic and it doesn't cause problems. I think it could be fine-tuned later.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 27, 2024
@silverwind
Copy link
Member

silverwind commented Jun 27, 2024 •
edited
Loading

Not sure if vertical alignment of first line is easily possible because the text can be multiline.

@wxiaoguang wxiaoguang enabled auto-merge (squash) June 27, 2024 13:43
@wxiaoguang wxiaoguang merged commit c1fe6fb into go-gitea:main Jun 27, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jun 27, 2024
@wxiaoguang wxiaoguang deleted the fix-toast branch June 27, 2024 13:59
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 28, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
b9bbd74 
* giteaofficial/main:
  Fix avatar radius problem on the new issue page (go-gitea#31506)
  Make toast support preventDuplicates (go-gitea#31501)
  Improve attachment upload methods (go-gitea#30513)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silverwind silverwind silverwind approved these changes

@yp05327 yp05327 yp05327 approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/js modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

Duplicate check and an auto disappear timer for toast
4 participants
@wxiaoguang @silverwind @yp05327 @GiteaBot