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

Remove all document.querySelector('.SiteLayout__main').innerHTML hacks. #2467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DrMeers
Copy link
Contributor

@DrMeers DrMeers commented Aug 14, 2024

I initially tried submitting #2464 for Hickenlooper but this was rejected.

I think if such a wholesale hack as replacing the entire HTML of the form is required, there are some deeper issues that should be investigated/resolved. I think this almost defeats the purpose of having step-by-step form filling YAML, at that point you could basically submit a custom payload to the server using a single javascript command rather than creating an arbitrary form and filling it out. These hacks are causing all kinds of trouble on our end including ReCAPTCHA failures.

I'm happy to help find the actual problems that these form-rewrite hacks have been instated to resolve, though so far I've found that each of these forms I've tested is succeeding if you remove the innerHTML javascript. Please share details of what problems are encountered for others if these lines are removed, and maybe we can figure out a more elegant/robust solution.

@j-ro
Copy link
Contributor

j-ro commented Aug 14, 2024

Unfortunately can't accept this -- it's required by the phantom system we use, which is the original (and at this point sole) point of this repo. Feel free to fork though!

@DrMeers
Copy link
Contributor Author

DrMeers commented Aug 14, 2024

are you happy to share what problems are you seeing if you remove these lines?

We're OK if not, and can just not collaborate on any of these ten members going forward (and any others that have this method applied to them in future), but seems a shame

@j-ro
Copy link
Contributor

j-ro commented Aug 14, 2024

We get failures when we try to fill them out. I don't have errors in front of me, but I'm sure they're specific to the phantom versions we're running, so not sure that's super fruitful to discuss unless you want to mimic our setup.

@DrMeers
Copy link
Contributor Author

DrMeers commented Aug 14, 2024

No worries, I'll detach our system from these ten members and try to keep an eye out for any document.querySelector('.SiteLayout__main').innerHTML updates in future and detach from those as well. Feel free to reach out if things change and you'd like help maintaining the YAML for these

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.

2 participants