Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

[Discussion] v3 API Changes #88

Open
crutchcorn opened this issue May 10, 2021 · 0 comments
Open

[Discussion] v3 API Changes #88

crutchcorn opened this issue May 10, 2021 · 0 comments

Comments

@crutchcorn
Copy link
Member

Now that we have robust integration testing and an example app on the way, I wanted to start thinking of what we can do for v3.

I have a few thoughts, and there's some existing work by @wcandillon on #49. I wasn't part of the project during that time, so I'm somewhat of an outsider looking in when it comes to the work in that PR. I figured instead of bothering @wcandillon for 1:1 explanations, we could open up the discussions to everyone and get some broader feedback on what we'd like to see in v3.

Potential Additions

useStyle/getStyle

First, some background. We'd like to remove useStylesheet/getStylesheet and replace them with something a bit more readable. For example, if we wanted to have a base set of styles (flex: 1) that's then overwritten in a specific media query flexDirection, we'd have to duplicate the base style:

    const styles = useStylesheet([
        {
            query: {orientation: "landscape"},
            style: {
                container: {
                    flex: 1,
                    flexDirection: "row"
                }
            }
        },
        {
            query: {orientation: "portrait"},
            style: {
                container: {
                    flex: 1, flexDirection: "column"
                }
            }
        }]);

Not only does this duplicate the base style, but it duplicates the key of the style container. This makes keeping styles consistent with each other significantly more difficult than need be.

One potential solution to this problem was suggested in #33. The idea being that we could have an API similar to Platform.select (from react-native core) as part of this library.

This is the API for Platform.select:

const styles = StyleSheet.create({
  container: {
    flex: 1,
    ...Platform.select({
      ios: {
        backgroundColor: 'red'
      },
      android: {
        backgroundColor: 'green'
      },
      default: {
        // other platforms, web for example
        backgroundColor: 'blue'
      }
    })
  }
});

However, we're unable to use the spread operator in a similar manner due to the reactive nature of Dimentions. Once a spread is ran, it cannot update the base object. The reason Platform is able to get away with this is because you will never have an instance where your app is running on one platform and need to be refreshed to another platform.

Because of this, we're thinking of a hybrid approach. Something that has the same readability advantages of Platform.select, but is reactive to window size changes for applications like those on the web.

  import {useStyle} from 'react-native-responsive-ui';

  const styles = useStyle(cond => ({
    container: {
        flex: 1,
        cond(
            { orientation: "landscape" },
            { flexDirection: "row" }
        ),
        cond(
            { orientation: "portrait" },
            { flexDirection: "column" }
        ),
    }
  }));

Altenatively, if we didn't want to use hooks, we could use:

  import {getStyle} from 'react-native-responsive-ui';

   const styles = getStyle(dimensions.get("window"), cond => ({
        container: {
            flex: 1,
            ...cond(
                {orientation: "landscape"},
                {flexDirection: "row"}
            ),
            ...cond(
                {orientation: "portrait"},
                {flexDirection: "column"}
            ),
        }
    }));

Bikeshed:

Do we want to call this useStyle? We could also name it:

  • useResponsiveStyle
  • Something else

We don't need to make cond an arrow function. We could instead have:

import {useStyle, cond} from 'react-native-responsive-ui';
 
const styles = useStyle({
  container: {
        flex: 1,
        cond(
            { orientation: "landscape" },
            { flexDirection: "row" }
        ),
        cond(
            { orientation: "portrait" },
            { flexDirection: "column" }
        ),
    }
});

However, we couldn't do the same for getStyle due to the lack of hooks. Because of this, I'm less in favor of this API, but a good argument could easily change my mind

DimensionsContext

This would allow class components to more easily get the value from useDimentions, and enable potential future APIs without requiring breaking changes.

Pros:

  • If we end up needing context in the future, we don't need to have a breaking release

Downsides:

Bikeshed:
Currently, none of our APIs require this context for usage. However, to simplify our documentation, should we simply suggest users always use DimentionsProvider in their apps?

Potential Removals

ResponsiveComponent abstract class

As it's removed from #49, it seems like something that both @wcandillon and myself seem to want to remove from the codebase. It's easily recreated in codebases that require it (especially if we add the context), isn't used internally in our codebase, and it seems like the larger React community is migrating away from class components

isInInterval function

While I don't plan on removing this from the codebase, I would like to remove the export that's set on the function currently. For a start, there's no documentation on the function, it's trivially re-implemented in codebases that needs it, and doesn't seem to match the purpose of our codebase

mediaQuery function

This is talking about the mediaQuery function documented in the README, not the MediaQuery component. That component isn't going anywhere and is part of what I consider our core API

This is another scenario where we'd potentially keep the code in our codebase but remove the export that exposes it publically. While this is documented in the README currently, it seems like it's been removed from #49's README. Further, it feels like it's exposing internal implementation and I'm admittedly having a difficult time thinking of how it would be utilized in an app. It's also easily confused with the component of the same name (different casing), which isn't ideal.

We're planning on breaking changes to the mediaQuery function regardless (namely, switching the order of props around for better readability), so removing the export and it's external usage would allow us to do that without documenting the change of props externally

useStylesheet/getStylesheet

See useStyle for more

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

No branches or pull requests

1 participant