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

Polyfill the window.customElements #23592

Merged
merged 2 commits into from Mar 20, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 20, 2023

Related: #23590

Reference: https://github.com/webcomponents/polyfills/tree/master/packages/webcomponentsjs

It seems that some browsers don't support customElements.

The Custom Elements would help a lot for Gitea's UI problems, including:

  • <span class="js-pretty-number">
  • <time data-format>

So it's worth get polyfill.

web_src/js/webcomponents/GiteaOriginUrl.js Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 20, 2023
Co-authored-by: delvh <dev.lh@web.de>
@delvh delvh added type/bug outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels Mar 20, 2023
@delvh delvh added this to the 1.20.0 milestone Mar 20, 2023
@silverwind
Copy link
Member

I don't think it's worth it. Any browser released in the last 5 years should have it. If such an ancient browser is used, I assume more stuff will be broken.

Also, the place where you polyfill is incorrect, it should be a separate file so that if more components are added, you avoid the double import.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 20, 2023

But we ever spent more time on older browsers, like this one:

Hmm ... same reporter in comment ....


About "Also, the place where you polyfill is incorrect, it should be a separate file so that if more components are added, you avoid the double import."

I know. But, it's not worth to make it that complex (there is only one file at the moment). Next time, when adding more components, there will be definitely a separate index file for all components (I promise)

@silverwind
Copy link
Member

silverwind commented Mar 20, 2023

PaleMoon only got it very recently, so yeah probably needed.

Can you just move the polyfill to webcomponents/polyfill.js and import that file in the component script?

@wxiaoguang
Copy link
Contributor Author

Can you just move the polyfill to webcomponents/polyfill.js and import that file in the actualy component script?

I am not sure whether I could do that as your expectation, could you help to make that change? You have the writer access to my fork.

@silverwind
Copy link
Member

Actually a index.js solution would be best, yeah.

@silverwind
Copy link
Member

I'm not sure if that polyfill is going to work because it seems it does not support extending. There is another one around that does.

@wxiaoguang
Copy link
Contributor Author

I have checked, and we do not need extend builtin elements (maybe we never want to do so).

Since webcomponentsjs seems to be the "official" one, so I choose it.

@silverwind
Copy link
Member

I think we need the other polyfill because we do class extends HTMLElement which is extending a built-in. You could just try in PaleMoon 32.0.1 to confirm whether the polyfill actually works.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 20, 2023

I think we need the other polyfill because we do class extends HTMLElement which is extending a built-in. You could just try in PaleMoon 32.0.1 to confirm whether the polyfill actually works.

Checked, it really works.

The fact is: our component never depends on the builtin element's behavior, so we are not extending them. class extends HTMLElement means nothing with the builtin element.

See my screenshot: the right one is with the polyfill (I also added a temp component on the page to show it works).

image

Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

I guess it's okay as short-term fix. When we add another web component, we need to move the polyfill.

@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 Mar 20, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 20, 2023
@lunny lunny merged commit 529bac1 into go-gitea:main Mar 20, 2023
2 checks passed
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 20, 2023
@GiteaBot
Copy link
Contributor

I was unable to create a backport for 1.19, please send one manually. 🍵

@GiteaBot GiteaBot added the backport/manual No power to the bots! Create your backport yourself! label Mar 20, 2023
@wxiaoguang wxiaoguang deleted the polyfill-custom-elements branch March 20, 2023 15:28
@lunny lunny added the backport/done All backports for this PR have been created label Mar 20, 2023
KN4CK3R pushed a commit that referenced this pull request Mar 20, 2023
Backport #23592

Close #23590

It seems that some browsers don't support customElements
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 21, 2023
* upstream/main:
  Replace a few fontawesome icons with svg (go-gitea#23602)
  Fix pagination on `/notifications/watching` (go-gitea#23564)
  Fix `.locale.Tr` function not found in delete modal (go-gitea#23468)
  fix submodule is nil panic (go-gitea#23588)
  `Publish Review` buttons should indicate why they are disabled (go-gitea#23598)
  Improve template error reporting (go-gitea#23396)
  Polyfill the window.customElements (go-gitea#23592)
  Add CHANGELOG for 1.19.0 (go-gitea#23583)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/manual No power to the bots! Create your backport yourself! lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/backport/v1.19 This PR should be backported to Gitea 1.19 type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants