Skip to content

Conversation

@thymikee
Copy link

@thymikee thymikee commented Dec 1, 2020

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍

Summary

Wanted to tackle #605 which was described as a regression likely when merging upstream virtualized lists changes. However, as I tested this through it turned out that it's the styling override that's to blame, not the actual functionality, which seem to work as intended.

Fixes #605

Changelog

[macOS] [Fixed] - Fix selection highlight regression in RNTester list

Test Plan

Screen Recording 2020-12-01 at 10 42 26

This color doesn't look great in dark mode tho. But I didn't want to diverge too much from the original RNTester. Suggestions welcome.

image

Microsoft Reviewers: Open in CodeFlow

@thymikee thymikee requested a review from alloy as a code owner December 1, 2020 09:48
@ghost ghost added Area: List labels Dec 1, 2020
Copy link
Member

@alloy alloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👌

As for dark theme, that's a larger work item that applies to all of RNTester and should probably start upstream. If you’re up for that do please have a go at it 🙏

<View
style={[
rowStyle, // TODO(macOS ISS#2323203)
{backgroundColor: theme.SystemBackgroundColor},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are the right colours brought in by theme.SystemBackgroundColor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list has properly being changed when switching dark mode on/off so I guess so? Because it worked as expected to me, I didn't really investigated what's under theme.SystemBackgroundColor really

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough 👍

@alloy
Copy link
Member

alloy commented Dec 2, 2020

I’m re-running the failed CI task.

Copy link
Member

@alloy alloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aight, all green. Thanks for your contribution, @thymikee! 🥇

@alloy
Copy link
Member

alloy commented Dec 2, 2020

@chrisglein Ooh, I see the bot propagates labels from the issue that a PR fixes; nifty! However, seeing as this only applies to RNTester I don’t think there’s a real need to backport this, or do you think differently?

EDIT: I realise that the determination that this only applies to RNTester came after filing the issue. Had it been a different underlying cause then it would have totally made sense to backport, but seeing as that’s not the case I think we’re in alignment in this not needing backporting work at this time.

@thymikee
Copy link
Author

thymikee commented Dec 2, 2020

Thanks for reviewing this so quickly! Nice contributing experience 😊

@alloy alloy merged commit bd2cbcd into microsoft:master Dec 2, 2020
@thymikee thymikee deleted the macos/fix-rntester-list-highlight branch December 2, 2020 13:51
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.

List items are not showing selection highlight color

2 participants