Conversation
59bf976 to
97d4852
Compare
efa6ded to
c598bf4
Compare
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Just reviewing the WidgetStateInputBorder migration before doing a separate review of the mappers. That maybe should have been it's own PR. Code LGTM. Some doc nits, and I think there needs to be tests specific to the new class
MitchellGoodwin
left a comment
There was a problem hiding this comment.
Mapper code LGTM, no nits there.
There was a problem hiding this comment.
@MitchellGoodwin thanks so much for the review! You're right that I should have added the mapper class in a separate PR first before doing the WidgetStateInputBorder stuff. Many thanks for looking over the whole thing regardless 🙏
5a20f70 to
5a97c7b
Compare
|
I've updated this pull request so it only includes the No rush on the review—I heard there's a lot of internal training going on this week. |
MitchellGoodwin
left a comment
There was a problem hiding this comment.
LGTM! Thank you very much!
|
@MitchellGoodwin thanks to you for the review! I wanted to ask a question before landing this one… I've been thinking about potentially cherry-picking this change into the upcoming stable release:
Would this be worth pursuing? |
As this wouldn't be a breaking change with .fromMap() and typos in the documentation aren't critical, I don't think there's a good chance of it being cherry picked. Cherry picking is expensive on our current infrastructure team, and is usually reserved for p1/p0 issues. You can ask around in the discord for opinions from people more closely involved with releases, but I don't think it's likely to go through. Last release there were some changes that didn't make the cut which I really wanted to get through but wasn't able to 😅 |
Ah, this is good to know. Thanks! |
This class looks like it _should_ give valid equality checks:
```dart
class _WidgetStateColorMapper extends _WidgetStateColorTransparent {
const _WidgetStateColorMapper(this.map);
final WidgetStateMap<Color> map;
_WidgetStateMapper<Color> get _mapper => _WidgetStateMapper<Color>(map);
@OverRide
Color resolve(Set<WidgetState> states) => _mapper.resolve(states);
@OverRide
bool operator ==(Object other) {
return other is _WidgetStateColorMapper && other.map == map;
}
@OverRide
int get hashCode => map.hashCode;
}
```
But I made a mistake: I should have used `other._mapper == _mapper` and `_mapper.hashCode`.
<br>
It'd be pretty nice if no copy-pasting was needed in the first place:
```dart
class _WidgetStateColorMapper extends WidgetStateMapper<Color> implements WidgetStateColor {
const _WidgetStateColorMapper(super.map);
}
```
<br><br>
This PR makes `WidgetStateMapper` a public class, similar to `WidgetStatePropertyAll`. The new API, when used incorrectly, will `throw` with a detailed error message, rather than silently returning irrelevant values.
**Changes** - Add `WidgetStateInputBorder` class, with `.resolveWith()` and `.fromMap()` constructors - Deprecate `MaterialStateOutlineInputBorder` and `MaterialStateUnderlineInputBorder` and provide data-driven fixes <br> **Other changes** based on #154972 (review) - Fix documentation copy-paste typo ("OutlinedBorder" � "InputBorder") - Add test to ensure borders are painted correctly - Add DartPad sample & relevant test
**Changes** - Add `WidgetStateInputBorder` class, with `.resolveWith()` and `.fromMap()` constructors - Deprecate `MaterialStateOutlineInputBorder` and `MaterialStateUnderlineInputBorder` and provide data-driven fixes <br> **Other changes** based on flutter#154972 (review) - Fix documentation copy-paste typo ("OutlinedBorder" � "InputBorder") - Add test to ensure borders are painted correctly - Add DartPad sample & relevant test
This class looks like it should give valid equality checks:
But I made a mistake: I should have used
other._mapper == _mapperand_mapper.hashCode.It'd be pretty nice if no copy-pasting was needed in the first place:
This PR makes
WidgetStateMappera public class, similar toWidgetStatePropertyAll. The new API, when used incorrectly, willthrowwith a detailed error message, rather than silently returning irrelevant values.