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

fix: isDark() utility can receive null value #3774

Merged
merged 4 commits into from
Apr 22, 2023
Merged

Conversation

exside
Copy link
Contributor

@exside exside commented Mar 23, 2023

Fixes #3764

Changes proposed in this pull request:
Shouldn't be many, was mainly an edge case when there were NULL values in the db that would crash this method.

Necessity

  • Has the problem that is being solved here been clearly explained?
    Well, it doesn't crash anymore =)
  • If applicable, have various options for solving this problem been considered?
    Decided to not make the method too fancypants for performance reasons (e.g. no regex checks for valid hex colors etc.)
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

exside added 2 commits March 23, 2023 15:08
Add early return if input looks fishy, minor refactoring and improvements of the entire method.
@exside exside requested a review from a team as a code owner March 23, 2023 14:18
@imorland imorland added javascript Pull requests that update Javascript code type/bug type/cleanup labels Mar 23, 2023
@imorland imorland added this to the 1.8 milestone Mar 23, 2023
Copy link
Member

@SychO9 SychO9 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!

framework/core/js/src/common/utils/isDark.ts Outdated Show resolved Hide resolved
framework/core/js/src/common/utils/isDark.ts Outdated Show resolved Hide resolved
@SychO9 SychO9 changed the title Fix 3764: Improve isDark() utility function fix: isDark() utility can receive null value Mar 25, 2023
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 requested a review from imorland April 21, 2023 20:02
@imorland imorland merged commit f8577c8 into flarum:main Apr 22, 2023
@github-actions github-actions bot mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/bug type/cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isDark throws an error and the app crashes if a color is set to NULL in the database
4 participants