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

aria-haspopup no-incompatible-type-binding false alarm #175

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

joekukish
Copy link

@joekukish joekukish commented Jun 9, 2021

Fixes #142

  • Updated underlying vscode-web-customdata that exposes the type for aria-haspopup.
  • Added test to make sure setting a non-boolean value is correct.

* Updated underlying vscode-web-customdata that exposes the type for aria-haspopup.
* Added test to make sure setting a non-boolean value is correct.
@joekukish
Copy link
Author

@justinfagnani @43081j Any chance you can have a look at this?

* Removed custom rule since we are not testing boolean attributes.
Copy link
Contributor

@43081j 43081j left a comment

Choose a reason for hiding this comment

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

Assuming CI passes, looks good to me. we need justin or peter to approve it too though

@justinfagnani
Copy link
Collaborator

@joekukish LGTM, thanks! I had landed a change to the no-incompatible-type-binding test so there's a conflict. Can you merge or rebase?

@joekukish
Copy link
Author

@justinfagnani I've rebased this branch and resolved conflicts. Please have a look!

@justinfagnani
Copy link
Collaborator

Thanks @joekukish !

One last thing - we need a changelog entry for this.

@brentswisher
Copy link

👋 @joekukish @justinfagnani We just ran across this issue in making some updates to https://github.com/ithaka/pharos and I was wondering if there was any update here? Would be happy to pick up the mantle to get this fix in the next version if that would be helpful, but hate to take something over as it looks like it's just a missing changelog that needs to be added?

@justinfagnani
Copy link
Collaborator

@brentswisher sorry this one languished. Yeah, it's just missing a changelog. I think I couldn't commit to the PR branch so would have to make a new one. @joekukish if you can, great, or @brentswisher could take this. Thanks everyone!

@brentswisher
Copy link

@justinfagnani I started looking into this, and found it appears to be working in the source code on the latest version of master.

I see the package is actively maintained with updates in the last few weeks/months, but a new release hasn't been cut since July of 2020. Am I misunderstanding something, it seems like there is a lot or work that has been done that isn't getting packaged up for consumers?

@daneah
Copy link

daneah commented Oct 17, 2023

Hey @justinfagnani, I think @brentswisher and I could work on getting a fresh PR together with a changelog. Do you think that will position you well for a new release? If there're other things outstanding we might be able to help as well; let us know.

@brentswisher
Copy link

Release v2.0.1 looks to have landed a fix for this as well, this PR can likely be closed now.

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

Successfully merging this pull request may close these issues.

aria-haspopup no-incompatible-type-binding false alarm
5 participants