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

Common API tools to get Sunrise bills only; display Sunrise bills only in Homepage #48

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

agreffard
Copy link
Contributor

@agreffard agreffard commented May 22, 2024

  • refacto to move tools to get Sunrise-only bills to be reused on Advanced + Home pages
  • Use id to display only Sunrise bills on the Home page

This PR is probably conflicting your work @derekmiranda ; this is a proposition, feel free to dismss it.
Or if you think this is going in the right direction we can discuss tomorrow how to integrate the filters work on it!

image

derekmiranda
derekmiranda previously approved these changes May 22, 2024
Copy link
Contributor

@derekmiranda derekmiranda left a comment

Choose a reason for hiding this comment

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

looks fine to me! i can pivot my work so it gels with this change

Copy link
Contributor

@elliott-king elliott-king left a comment

Choose a reason for hiding this comment

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

This doesn't handle filters atm, so it will break existing behavior. I don't think this should be merged as-is.
@agreffard mentioned that he intends to add that? Alternatively, I'm working on a change for the other branch to fix this.

@agreffard
Copy link
Contributor Author

agreffard commented May 23, 2024

I think @derekmiranda sad he will have a look. I think it's a matter of combining the 2 term values (one for the filters, one for the sunrse bill selection). Let me know if it's not that easy @derekmiranda we can try to find a moment for pair-programming

@derekmiranda
Copy link
Contributor

@agreffard @elliott-king i committed a change to integrate the searchTerm w/ legislator name and bill status. tho i dont think it's quite filtering by those filters, the search term gets applied when i check the api requests

Copy link
Contributor

@elliott-king elliott-king left a comment

Choose a reason for hiding this comment

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

I'd be okay with just approving #40, approving this, merging both, and check to see they work.
I have my own fix locally, but it is a lot bigger and this does the trick afaik

Comment on lines 72 to 74
const term = encodeURIComponent(
`basePrintNo:(${page.map((printNo) => `+${printNo}`).join(' OR ')}) ${addedTerm}`
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const term = encodeURIComponent(
`basePrintNo:(${page.map((printNo) => `+${printNo}`).join(' OR ')}) ${addedTerm}`
);
let term = `printNo:(${page.map((printNo) => `${printNo}`).join(' OR ')}) ${addedTerm}`;
// term = encodeURIComponent(term);
// console.log('term', term);
  • I'm having issues w/ HTML encoding (request only works w/out)
  • why the plus in +${basePrintNo} ?
  • for some reason, I'm also getting versioned bill (this may be my fault, maybe we should use printNo instead of basePrintNo ..?)

Otherwise, it seems to work to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

the plus was an attempt to get it working with the legislator and status filters; i thought that applying some more specificity with the print num would let those filters actually filter by legislator or status. i dont think it's doing anything so ill remove

i'll also use printNo instead of basePrintNo, if it adds more specificity. im guessing "versioned bills" means non-canonical versions of the main bill?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this file is here.

Copy link
Contributor

Choose a reason for hiding this comment

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

that shouldnt be there ill remove it

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.

3 participants