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

Fixes for search warnings errors and addressing issue 114 #118

Conversation

deepend-tildeclub
Copy link
Contributor

renamed this. that comma was causing issues with my local machines git. Hopefully this doesn't mess everything up.

@deepend-tildeclub
Copy link
Contributor Author

There now my git should be functioning as intended. Sorry about all the issues. I'm not the best at git.

@RedDragonWebDesign
Copy link
Owner

RedDragonWebDesign commented Jan 27, 2024

Sorry about all the issues. I'm not the best at git.

Don't worry, you're not the only one. I hate git. So unintuitive for beginners. Desperately needs a GUI.

@deepend-tildeclub
Copy link
Contributor Author

Sorry about all the issues. I'm not the best at git.

Don't worry, you're not the only one. I hate git. So unintuitive for beginners. Desperately needs a GUI.

I'm ok with no GUI but it could at least be a little less convoluted. I run tilde.club which mostly centers around the command line. so I'm generally good with that.

@deepend-tildeclub
Copy link
Contributor Author

Is this one also too large? Or can this one be manageable.

@RedDragonWebDesign
Copy link
Owner

Related #114

@RedDragonWebDesign
Copy link
Owner

Yes, can you split this one too? One patch for the #114 feature, and one patch per page of error fixes. So if you fix all the errors on the search results page, that could be one patch. And if you fix all the errors on the advanced search page, that could be an additional patch.

Adding public $description and public $prefillValues to Form.php looks suspicious since they are never used. And I can't tell what error they're trying to fix. Splitting into more patches that are clear about what errors the patch is fixing would help review this kind of code.

Thank you for being flexible and responsive to my comments. I appreciate it.

@deepend-tildeclub
Copy link
Contributor Author

Yes, can you split this one too? One patch for the #114 feature, and one patch per page of error fixes. So if you fix all the errors on the search results page, that could be one patch. And if you fix all the errors on the advanced search page, that could be an additional patch.

Adding public $description and public $prefillValues to Form.php looks suspicious since they are never used. And I can't tell what error they're trying to fix. Splitting into more patches that are clear about what errors the patch is fixing would help review this kind of code.

Thank you for being flexible and responsive to my comments. I appreciate it.

I think its possible that only relevant part of this PR is the 114 portion of it.. Since when I made this one I was still having issues with my git that I didn't realize. Now things are good so we push the split the 114 portion of it out and throw the rest away. Hopefully the other PR's I made have a better chance of making it somewhere since I focused on making them as simple and clean as possible. :)

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