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

Make ContextPopup stateless, improve fetching logic #31181

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented May 30, 2024

  1. ContextPopup is now a stateless component, accepting only props to render the popup
  2. Loading state is gone, the requests are fast enough to not need a loading state
  3. Error state is gone, we don't need to show any errors on this, imho
  4. Popups now have the tooltip role which enables singleton behaviour, so multiple such popups won't show at the same time (happens when links are close together)
  5. Increased the interactiveBorder to 15px
  6. Tooltip init is now lazy
  7. Repo link in tooltip is now clickable

Fixes: #31161
Helps with: #30275

Popup over existing issue:

image

Nonexisting issue does not show anything:

image

I think this is a nice bugfix for 1.22.

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 30, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 30, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/js labels May 30, 2024
@silverwind silverwind changed the title Rewrite context popup to stateless Make ContextPopup stateless May 30, 2024
@silverwind silverwind added the backport/v1.22 This PR should be backported to Gitea 1.22 label May 30, 2024
@silverwind silverwind marked this pull request as draft May 30, 2024 14:50
@silverwind
Copy link
Member Author

silverwind commented May 31, 2024

In 434287a I added another data attribute to track the loading state, ensuring this will never issue two GET requests at the same time for the same link, no matter which DOM events trigger on the link.

@silverwind silverwind changed the title Make ContextPopup stateless Make ContextPopup stateless, improve fetching logic Jun 3, 2024
@yp05327 yp05327 added this to the 1.23.0 milestone Jun 4, 2024
@wxiaoguang
Copy link
Contributor

Sorry but I do not think the change is good enough.

When you'd like to reuse an existing instance, all states should be handled carefully. For example

  • Hover on #1, then #1 is loading, then hover to #2, should #2 be loading immediately? (I do not see so in this PR)
  • Hover on #1, then #1 fails due to temp network error. Then re-hover on #1, will it reload?
  • Hover on #1, then #1 loads, then #1 changes, then re-hover on #1, will it reload?

I do not think it's worth to force it only use one instance.

I strongly prefer the old approach, which is simple enough and doesn't really cause any problem.

@silverwind
Copy link
Member Author

silverwind commented Jun 4, 2024

I think single instance is preferable because with the increased interactivity border, it's possible to have situations like this:

image

As for the other questions, I will check. Of course the last hovered instance should always win.

@wxiaoguang
Copy link
Contributor

I think single instance is preferable because with the increased interactivity border, it's possible to have situations like this:

I wouldn't say it is a serious problem, and it could be fixed by add some "hiding" code.

@silverwind
Copy link
Member Author

silverwind commented Jun 10, 2024

The problem with double tooltip can also be seen with the commit status in commit list. I think it could be fixed by adding a singleton="tooltip" during creation, so that only one instance per singleton can show. Currently this mechanism is active when role is tooltip but I think it may need to become more flexible.

@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 12, 2024
@silverwind silverwind marked this pull request as draft June 12, 2024 22:06
@wxiaoguang wxiaoguang removed the backport/v1.22 This PR should be backported to Gitea 1.22 label Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code modifies/js modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The PRs link in release page will get network error
5 participants