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

[5.3] Remove TODO joomla-alert.scss #44274

Merged
merged 4 commits into from
Oct 18, 2024

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Oct 16, 2024

While reviewing existing @todo I came across these two.

The first one is not needed as the variable has the same value The second one makes no sense

@coolcat-creations these were your todo comments from #42986 - have I missed something or can they simply be removed

Testing Instructions

code review

While reviewing existing @todo I came across these two.

The first one is not needed as the variable has the same value
The second one makes no sense

@coolcat-creations these where your todo comments - have I missed something or can they simply be removed

Signed-off-by: BrianTeeman <brian@teeman.net>
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.3-dev labels Oct 16, 2024
Resolve todo as we only support php8 now.

This should probably have been done when I did joomla#41855

Original PR where this check was added joomla#28694

Signed-off-by: BrianTeeman <brian@teeman.net>
This reverts commit 485596d.
@coolcat-creations
Copy link
Contributor

Sorry I don't understand these TODOS anymore ;(
next time I need to write why or so... I think it can be removed.

@brianteeman
Copy link
Contributor Author

Thanks for confirming. You are not alone of the 300+ todo many of them make no sense at all and have been there for a very very long time

@fgsw
Copy link

fgsw commented Oct 18, 2024

@brianteeman I wanted to test by download prepbuild package as it has the "NPM Resource Changed" label but could not find build/media_source/templates/administrator/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss in the package.

@brianteeman
Copy link
Contributor Author

@fgsw files in the build/ folder and not distributed. They are used to create the css files found in the media/ folder

@fgsw
Copy link

fgsw commented Oct 18, 2024

Thanks for information that the pull request can't get a test by prebuilt package.

@rdeutz
Copy link
Contributor

rdeutz commented Oct 18, 2024

I agree that todos are often not useful, I will put this on the maintainers agenda, I think a todo should contain a version we have to work on the todo, a description and maybe a reason.

@brianteeman
Copy link
Contributor Author

@rdeutz in this specific case the person who wrote the todo states she doesnt know why

@rdeutz
Copy link
Contributor

rdeutz commented Oct 18, 2024

understandable after months/years, if we force people to write more about the todo changes are goodl that they know why

@rdeutz rdeutz merged commit 2e5a93b into joomla:5.3-dev Oct 18, 2024
2 of 3 checks passed
@rdeutz
Copy link
Contributor

rdeutz commented Oct 18, 2024

Thanks

@brianteeman
Copy link
Contributor Author

understandable after months/years, if we force people to write more about the todo changes are goodl that they know why

hence my comment on your todo yesterday ;)

@brianteeman brianteeman deleted the state_warning_todo branch October 18, 2024 08:48
@richard67 richard67 added this to the Joomla! 5.3.0 milestone Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.3-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants