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

tonalSubheading in dialog #80

Merged
merged 2 commits into from
Mar 16, 2024
Merged

tonalSubheading in dialog #80

merged 2 commits into from
Mar 16, 2024

Conversation

geovaniprodata
Copy link
Contributor

You guys forgot to insert tonalSubheading when use show color picker dialog, the ColorPicker itself use the parameter, but the dialog only declares him.

@rydmike
Copy link
Owner

rydmike commented Mar 16, 2024

Hi @geovaniprodata, this indeed a correct observation of minor bug. Good find, thank you! 😄

It has probably been left unnoticed for this long since very few actually uses the convenience function showColorPickerDialog. It should of course be fixed.

As you can see, your PR fails checks since it does not use dart format, which allows only max width 80 chars. Which is needed for packages on pub. Pub scoring check that packages uses dart format, and only with 80 chars line length, it does not understand anything else. If packages on pub do not pass dart format check loose some score points. Yes 80 chars is a silly low number for Flutter, I agree, but it it was pub requires for best score and there is no way to enforce other than 80 chars on project level.

Usually I would not approve a PR that has a lot of changes in updated file not related to the fix, one previously omitted line in this case. But since I can see it is the only thing that has actually been changed, I am going to merge it anyway.

The placement is also wrong, it should come after the subheading. I'm not going to nitpick about that either, I'll move it after merging.

Also need to create an issue for the PR that it fixes.

We should add tests for this particular dialog function and prop too, but that is something is a bit lacking anyway. I think I have the test for the ColorPicker that does it, but not via this showColorPickerDialog so I will add a case for that too.

@rydmike
Copy link
Owner

rydmike commented Mar 16, 2024

Added issue for this fix: #81

@rydmike
Copy link
Owner

rydmike commented Mar 16, 2024

I also added to the Web demo a dialog that actually uses future function showColorPickerDialog. The existing demo only used "raw" CololPicker and its showPickerDialog method. And yes the tonal heading will not show up in that dialog dues to this issue 😄

@rydmike
Copy link
Owner

rydmike commented Mar 16, 2024

FIX: #81

@rydmike rydmike merged commit 4c39804 into rydmike:master Mar 16, 2024
1 check failed
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.

None yet

2 participants