[Discussion] v3 API Changes #88
Description
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:
- Easily implementable in client-side
- Not currently used
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 theMediaQuery
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