Skip to content

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Jul 14, 2025

"form" has an edge case: its <input name=action> element overwrites the action property, we can only set attribute.

This PR makes assignElementProperty can handle such case, and add more tests

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 14, 2025
@wxiaoguang wxiaoguang added the backport/v1.24 This PR should be backported to Gitea 1.24 label Jul 14, 2025
@wxiaoguang wxiaoguang added this to the 1.25.0 milestone Jul 14, 2025
setAttribute: (name: string, value: string) => void;
} & Record<string, any>

export function assignElementProperty(el: ElementWithAssignableProperties, kebabName: string, val: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not Element or HTMLElement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it needs "any":

Image

Copy link
Member

@silverwind silverwind Jul 14, 2025

Choose a reason for hiding this comment

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

Shouldn't keyof typeof Element work for the property access?

  const camelizedName: keyof typeof Element = camelize(kebabName);

Copy link
Member

@silverwind silverwind Jul 14, 2025

Choose a reason for hiding this comment

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

If not, use (el as any)[camelizedName] = ... to assign. Better do dirty stuff within a function than to expose any to consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a clearer solution.

Feel free to edit this PR directly with your solution.

@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 Jul 14, 2025
@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 Jul 14, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 14, 2025
@lunny lunny merged commit 692c90e into go-gitea:main Jul 14, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 14, 2025
@wxiaoguang wxiaoguang deleted the fix-form-property branch July 15, 2025 00:11
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Jul 15, 2025
"form" has an edge case: its `<input name=action>` element overwrites
the `action` property, we can only set attribute.

This PR makes `assignElementProperty` can handle such case, and add more
tests
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Jul 15, 2025
wxiaoguang added a commit that referenced this pull request Jul 15, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 16, 2025
* giteaofficial/main:
  Send email on Workflow Run Success/Failure (go-gitea#34982)
  [skip ci] Updated translations via Crowdin
  Replace `poetry` with `uv` (go-gitea#35084)
  nix flake update (go-gitea#35085)
  Use monospace font in PR command line instructions (go-gitea#35074)
  Add gitignore rules to exclude LLM instruction files (go-gitea#35076)
  [skip ci] Updated translations via Crowdin
  Fix form property assignment edge case (go-gitea#35073)
  Improve submodule relative path handling (go-gitea#35056)
  Fixed all grammatical errors in locale_en-US.ini (go-gitea#35053)
  UI: add hover background to table rows in user and repo admin page (go-gitea#35072)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done All backports for this PR have been created backport/v1.24 This PR should be backported to Gitea 1.24 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants