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

GM Notes Header Button #27

Closed
mouse0270 opened this issue Aug 10, 2023 · 14 comments
Closed

GM Notes Header Button #27

mouse0270 opened this issue Aug 10, 2023 · 14 comments

Comments

@mouse0270
Copy link
Contributor

Is there any particular reason you append the header button in the renderXYZ hook instead of registering it as a header button in the getXYZHeaderButtons hooks?

@bithir
Copy link
Owner

bithir commented Aug 10, 2023

No, no reason - it is was just made this way when I took over the project.

@mouse0270
Copy link
Contributor Author

If I make a pull request can we update it?

@bithir
Copy link
Owner

bithir commented Aug 10, 2023

Of course, I happily accept pull request. Can you detail what the advantages are?

@mouse0270
Copy link
Contributor Author

Using the getHeadersButtons hook is the Standard Foundry API method used for registering buttons in the header. This method also makes it so other modules who are getting and interaction with header buttons knows the button exists.

For example, my module WindowTabs, groups windows together and only shows buttons for the currently active tab. So if the button hasn't been registered as a header button, my module has no way to know it exists.

Basically, its the proper way to do it, and should only be circumvented if absolutely needed.

@bithir
Copy link
Owner

bithir commented Aug 11, 2023

Sounds good - looking forward to the PR :)

@bithir
Copy link
Owner

bithir commented Aug 17, 2023

I've been looking at the hook - but I can't see how I change the indicator on the button upon changes.
I must be missing something - I can't close and open the app to just update the header button.

So I would still need to listen to the render hook to do the update?

@mouse0270
Copy link
Contributor Author

mouse0270 commented Aug 17, 2023

Sorry I dont use this module, could you tell me what is changing about the button on the render hook?

I can test it out later today to figure it out myself, but I sadly have been swamped all week and haven't gotten around to this.

@bithir
Copy link
Owner

bithir commented Aug 17, 2023

The icon (and text) changes if you are looking at an item/actor/light/tile depending if they have a note or not.
Example: No note
image
Example: Have note
image

@bithir
Copy link
Owner

bithir commented Aug 17, 2023

I'm suspecting that this is the reason other modules are also doing it in the render hook. If the button depends on the underlying data, then using the button header seems quite involved, compared to just adding it in the render hook.

@mouse0270
Copy link
Contributor Author

Well, changing the icon, is easily enough, but the color is a littler tricker, but I can do it via css. I try and work on it later.

Impossible probably not, being tricky, probably so. haha

@bithir
Copy link
Owner

bithir commented Aug 18, 2023

The big issue is that the header button hook only triggers once (when the sheet is opened), so there is no possible way to change the icon/text if the underlying data changes.

I have another module which has a header button but this one does not change, where I will move over to use the button header hook.

@mouse0270
Copy link
Contributor Author

mouse0270 commented Aug 18, 2023

Made the pull request, which because also happens to appear to also add support for #1 because I am attaching to any application that has a .document

Though I don't use this module, so I may have missed something, I did basically zero testing, Just mostly made sure the button showed up and changed icon/color

@mouse0270
Copy link
Contributor Author

You should be able to close this issue and #1

@bithir
Copy link
Owner

bithir commented Aug 19, 2023

Resolved by #29

@bithir bithir closed this as completed Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants