Skip to content

Conversation

@HosamHasanRamadan
Copy link

@HosamHasanRamadan HosamHasanRamadan commented Jul 1, 2024

Fixes: #110319

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Jul 1, 2024
@Piinks Piinks requested a review from MitchellGoodwin July 3, 2024 18:18
@Piinks
Copy link
Contributor

Piinks commented Jul 3, 2024

@HosamHasanRamadan thanks for contributing! Welcome!
Can you add a test that verifies that this works as expected?

@HosamHasanRamadan
Copy link
Author

Hi @Piinks
I was trying to add tests to this change, but I didn't know where to start.

  • _getColumnWidth is private
  • the function depends on flutter_localizations which is not part of flutter package

Can you guide me through ?
Or if there is a similar test that I can read because I didn't find any test in date_picker_test.dart to old code.

@MitchellGoodwin
Copy link
Contributor

Another PR fixing a similar issue with a different implementation was filed here: #151128

@MitchellGoodwin
Copy link
Contributor

Or if there is a similar test that I can read because I didn't find any test in date_picker_test.dart to old code.

This is a tricky thing to test, the person in the other PR is having the same problem. It looks like the test needs to both override the localizations with one that triggers the issue and use a different font then what the tests use by default. The font used by the tests have a standard spacing that may not trigger this issue. See this comment and the one above it.

@HosamHasanRamadan
Copy link
Author

Thanks @MitchellGoodwin
Using Localizations.override fixes the localization problem and managed to render Arabic months.
Now I have problem with the font , it seems like monospaced font is used during test.
The problem isn't reproducible in this case.
How can I override the font is this situation?

@HosamHasanRamadan HosamHasanRamadan force-pushed the 110319_fix_cupertino_date_picker_column_width branch from 220fc8e to 747000f Compare July 6, 2024 15:58
@MitchellGoodwin
Copy link
Contributor

The other PR has the same issue, and looks like they were able to load Roboto. Link to comment on the section. I don't know enough about how the fonts load in the tests to know if it's a safe test though.

@HosamHasanRamadan
Copy link
Author

@MitchellGoodwin
I managed to add test without exposing any private members in CupertinoDatePicker file.
I used Local('pr') for locale and months values , I didn't manage to test it with Arabic values.

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@MitchellGoodwin
Copy link
Contributor

Going to close this in favor of #151128. Feel free to comment on that PR. If that one is closed for any reason, also feel free to reopen this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CupertinoDatePicker in Arabic showing months in wrong way

3 participants