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

[ios] InstructionView secondary row uses theme bg color #259

Conversation

michaelkirk
Copy link
Contributor

I noticed while working on the InstructionView that the secondaryStyle.backgroundColor is ignored, and instead is hardcoded as a transformation of the primaryStyle.backgroundColor.

I think using the secondaryStyle.backgroundColor is less surprising, so I've made that change.

Further, while working on that, I noticed that all the render tests were using a test style (TestingInstructionRowTheme) rather than the default styles, which has a couple shortcomings:

  1. it was using the same style for primary and secondary, giving an unrealistic look at how people will use these views
  2. darkMode testing wasn't possible
  3. Having a visually diff of the rendered style across the many permutations of view state with every PR "for free" seems super useful

So, I've change these tests to use the default styles rather than the test style. However, I'm assuming there was a good reason for introducing the Test style in the first place, and so if it's still desirable for some reason, we can revisit that part of the change. For context, it looks like it was added as part of the original InstructionView feature in #71

@michaelkirk michaelkirk force-pushed the mkirk/secondary-row-uses-theme-bg-color branch from f0dc683 to c2d7658 Compare September 20, 2024 00:59
Copy link
Collaborator

@Archdoog Archdoog left a comment

Choose a reason for hiding this comment

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

Look good. Thanks!

Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

LGTM; thanks!

Re: the question about using a test theme, I actually don't recall the exact reason for that. Though I do know a lot of similar changes have been made (ex: baking formatters rather than accepting defaults like most end users will) to make the tests reproducible. I can't detect any changes here that break that (and my machine is notoriously configured to clash with a lot of system defaults that will pass CI haha).

@ianthetechie ianthetechie merged commit 02c7dd6 into stadiamaps:main Sep 22, 2024
14 checks passed
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.

3 participants