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

Add tooltips to the item sheet traits #17685

Merged
merged 13 commits into from
Jan 31, 2025

Conversation

Kulkodar
Copy link
Contributor

@Kulkodar Kulkodar commented Dec 11, 2024

Adjusted createTagifyTraits to include the tag description and adjusted the tag template to remove the browser tool tip from tags. Side effect tags in creature sheets also show their tootip.
Closes #17491

2024-12-11.18-13-55.mp4

TODO:

  • cleanup code
  • test with tags not defined in CONFIG.PF2E.traitsDescriptions
  • hide browser tool tip

@Kulkodar Kulkodar marked this pull request as ready for review December 12, 2024 12:27
src/util/tags.ts Outdated
Comment on lines 72 to 83
return `<tag
contenteditable='false'
spellcheck='false'
tabIndex="${this.settings.a11y.focusableTags ? 0 : -1}"
class="${this.settings.classNames.tag}"
${this.getAttributes(tagData)}>
<x title='' class="${this.settings.classNames.tagX}" role='button' aria-label='remove tag'></x>
<div>
<span class="${this.settings.classNames.tagText}">${tagData[this.settings.tagTextProp] || tagData.value}</span>
</div>
</tag>`;
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

No inline HTML

Copy link
Contributor Author

@Kulkodar Kulkodar Dec 15, 2024

Choose a reason for hiding this comment

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

@stwlam i might need help since i'm not that familiar with type script.
As far as i can see i cant use renderTemplate since an async function can't be assigned to tag: string.
I could use createElements, but then i have to either extend the function to allow custom tags and attributes or change the custom tags to html compliant tags and adjust the styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stwlam fixed now with the help of @ricardoSlv.

@Kulkodar
Copy link
Contributor Author

i moved the html snipped into a getter function, i'm not sure if you meant that or that i should move it into a file.

@Kulkodar Kulkodar requested a review from stwlam December 15, 2024 10:03
@CarlosFdez
Copy link
Collaborator

CarlosFdez commented Dec 15, 2024

I assume stwlam means to use the dom functions to create elements. There's utility methods in src/util/dom.ts to reduce how verbose they are and make it play better with typescript as well.

@stwlam
Copy link
Collaborator

stwlam commented Dec 15, 2024

A template would probably be best in this case.

@ricardoSlv
Copy link

@Kulkodar Still wanna finish this? I believe I help you if you want 🙋‍♂️

@Kulkodar
Copy link
Contributor Author

@ricardoSlv Life got a little busy but yes a nudge in the right direction would be nice.

@Kulkodar Kulkodar marked this pull request as draft January 28, 2025 10:40
@Kulkodar Kulkodar marked this pull request as ready for review January 28, 2025 15:04
src/util/tags.ts Outdated
tag.spellcheck = false;
tag.tabIndex = this.settings.a11y.focusableTags ? 0 : -1;

Object.entries(tagData).forEach(([key, value]) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer actual for loops

@Kulkodar Kulkodar requested a review from CarlosFdez January 29, 2025 11:45
@CarlosFdez
Copy link
Collaborator

Out of curiousity, before I check it out locally and start seeing if everything still works ok, why the CSS changes?

@Kulkodar
Copy link
Contributor Author

tagify has a css rule that apply on .tagify__tag>div that broke/changed the layout of the x button and tag itself since they are now divs instead of custom types.

@CarlosFdez
Copy link
Collaborator

Is there an issue continuing to use that custom tag?

@Kulkodar
Copy link
Contributor Author

createHTMLElement does not allow custom tags as far as i see and i am not sure if it is possible/ a good idea to extend/change createHTMLElement to allow custom tags.

@CarlosFdez
Copy link
Collaborator

CarlosFdez commented Jan 31, 2025

I'd like to tag in @stwlam but if we don't relax the typing, you can use document.createElement() or cast it.

I'd rather not chance inconsistencies or possible errors from changing the tag name. I think trying to keep the result as close as possible to what we already have is best.

@CarlosFdez
Copy link
Collaborator

Alright, looks fine to me. There's some noise in the data (__isValid etc), but the original wasn't clean to begin with. Thanks for the PR.

@CarlosFdez CarlosFdez changed the title Item sheet trait tooltip Add tooltips to the item sheet traits Jan 31, 2025
@CarlosFdez CarlosFdez merged commit ed97d9d into foundryvtt:master Jan 31, 2025
1 check passed
@Kulkodar
Copy link
Contributor Author

Kulkodar commented Feb 1, 2025

Thanks for taking your time

@Kulkodar Kulkodar deleted the item-sheet-trait-tooltip branch February 1, 2025 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Hover over trait tags on item sheets to show description tooltip
4 participants