Skip to content
This repository has been archived by the owner on Jul 2, 2024. It is now read-only.

Remove StyleProvider #29

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Remove StyleProvider #29

wants to merge 7 commits into from

Conversation

gksander
Copy link
Contributor

@gksander gksander commented Jul 6, 2022

What is this?

This PR removes the need to wrap your app in StyleProvider. The reason for this change is twofold:

  1. It's easier to setup if you don't have to wrap your app. I like that, and then dark mode just sort of "magically" works without the provider.
  2. I'm working toward a set of response utilities, where you can apply "classes" based on screen size. This will require a top-level WindowDimensions listener. I'd like to keep things as performant as possible, and not have to deal with Context values changing and triggering cascading re-renders.

This PR also adds a bunch of cleanup around tests.

@@ -1,15 +1,13 @@
import * as React from "react";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is just from the sample, was just fiddling with some things.

Comment on lines -6 to +5
return (
<StyleProvider>
<AppBody />
</StyleProvider>
);
return <AppBody />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No more Provider wrapper, woot.

Choose a reason for hiding this comment

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

Yay!

* - Used to hold state (like colorScheme preference), which can be updated from a single
* event listener, and have those updates emitted out to multiple hook-usages.
*/
export class SimpleStore<S> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

THE NEXT REDUX!

Jk, this is like the most barebones possible state-management solution. I like it because it's lean, and doesn't require Context.


type StyleProviderProps = {
colorScheme?: "light" | "dark" | "auto";
};

/**
* TODO: Deprecate this. No longer needed, but leaving for now.
*/
export const StyleProvider = ({

Choose a reason for hiding this comment

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

Can we use the following comment

/**
 * @deprecated This provider should not be used
 */

export class SimpleStore<InitialValue, OutputValue = InitialValue> {
#value!: OutputValue;
#ee = new SimpleEventEmitter<OutputValue>();
#transformer!: (val: InitialValue) => OutputValue;

Choose a reason for hiding this comment

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

first time ever seeing private class vars in ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just recently started using them, and then realized like half of the class vars I use should be considered private, and I love that there's now an easy way for declaring them as such.

@gksander
Copy link
Contributor Author

gksander commented Jul 6, 2022

Damn, just now seeing that RNW doesn't support Appearance.addChangeListener, so this is going to fail for RNW 😢

I'm torn on this. I really like this change to the API, but not supporting RNW is a real downside.

UPDATE!

Looks like Appearance.addChangeListener does exist, it just doesn't conform to RN's new API for change listeners (e.g., returning a remove method from the subscription). I've created a PR for RNW to support this.

Copy link

@carloskelly13 carloskelly13 left a comment

Choose a reason for hiding this comment

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

Just one comment, otherwise looks great.

@gksander gksander requested a review from carloskelly13 July 6, 2022 18:40
@atanaskanchev
Copy link

LGTM? 🥇

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants