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

Settings Saved Notice #177

Merged
merged 2 commits into from
Nov 13, 2024
Merged

Settings Saved Notice #177

merged 2 commits into from
Nov 13, 2024

Conversation

namithj
Copy link
Contributor

@namithj namithj commented Nov 12, 2024

Settings Saved Notice

Pull Request

What changed?

Settings Saved Notice

Why did it change?

Add a Settings Saved Notice for clarity

Did you fix any specific issues?

#155

CERTIFICATION

By opening this pull request, I do agree to abide by the Code of Conduct and be bound by the terms of the Contribution Guidelines in effect on the date and time of my contribution as proven by the revision information in GitHub.

Settings Saved Notice
@namithj namithj requested review from costdev and a team November 12, 2024 16:57
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @namithj! I've left a few small change requests in this one for multisite support, hook argument accuracy, and some simplicity, and also a larger suggestion - let me know your thoughts. 🙂

includes/class-admin-settings.php Outdated Show resolved Hide resolved
includes/class-admin-settings.php Outdated Show resolved Hide resolved
includes/class-admin-settings.php Outdated Show resolved Hide resolved
includes/class-admin-settings.php Outdated Show resolved Hide resolved
@namithj
Copy link
Contributor Author

namithj commented Nov 12, 2024

Merging the two _notice functions.

Combine admin notice functions
@namithj
Copy link
Contributor Author

namithj commented Nov 13, 2024

@costdev PR Updated

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @namithj!

@asirota
Copy link
Member

asirota commented Nov 13, 2024

Unit tests failed. Please review

@asirota asirota added the enhancement New feature or request label Nov 13, 2024
@asirota asirota added this to the Phase 1 milestone Nov 13, 2024
@asirota asirota linked an issue Nov 13, 2024 that may be closed by this pull request
@namithj
Copy link
Contributor Author

namithj commented Nov 13, 2024

Unit tests failed. Please review

Looks like Its got to do with w.org. https://wordpress.org/latest.tar.gz is giving 502 error.

@costdev
Copy link
Contributor

costdev commented Nov 13, 2024

Yeah it's most likely linked to the 502s.

There's nothing in the PR that should affect the existing tests. Nevertheless, I ran the test suite locally for Single Site and Multisite on PHP 7.4 and PHP 8.2 and can confirm that all tests are passing.

@asirota
Copy link
Member

asirota commented Nov 13, 2024

Yeah it's most likely linked to the 502s

What are the 502s?

@namithj
Copy link
Contributor Author

namithj commented Nov 13, 2024

W.org was down for 4-6 hours today due to some bad server config!

@asirota asirota merged commit 13d84ee into aspirepress:main Nov 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add a notice when settings are saved
3 participants