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

no-unused-styles: false positive with variable based style object selection #320

Open
soullivaneuh opened this issue Sep 9, 2022 · 7 comments

Comments

@soullivaneuh
Copy link

Considered this component having a variant prop with values white and primary, affecting the selection of the styles object:

import React, { FC } from 'react';
import {
  StyleSheet, KeyboardAvoidingView, Platform, View,
} from 'react-native';
import { SafeAreaView } from 'react-native-safe-area-context';
import { colors } from '@company/design';
import Constants from 'expo-constants';

interface Props {
  logonScreen?: boolean;
  variant?: 'white' | 'primary';
}

const styles = StyleSheet.create({
  safeArea: {
    flex: 1,
    backgroundColor: colors.white,
  },
  safeAreaLogon: {
    backgroundColor: 'white',
  },
  container: {
    flex: 1,
  },
  center: {
    justifyContent: 'center',
  },
});

const variants = {
  white: StyleSheet.create({
    container: {
      backgroundColor: 'white',
    },
  }),
  primary: StyleSheet.create({
    container: {
      backgroundColor: colors.primary,
    },
  }),
};

export const ScreenWrapper: FC<Props> = ({
  logonScreen, children, variant,
}) => {
  const variantStyle = variants[variant || 'white'];
  const viewStyle = [
    styles.container,
    variantStyle.container,
    ...logonScreen ? [styles.center] : [],
  ];

  return (
    <SafeAreaView
      style={[
        styles.safeArea,
        ...logonScreen ? [styles.safeAreaLogon] : [],
      ]}
    >
      <View style={viewStyle}>
        {
          Platform.OS === 'ios'
            ? (
              <KeyboardAvoidingView
                style={{ flex: 1 }}
                behavior="padding"
                keyboardVerticalOffset={Constants.statusBarHeight}
              >
                {children}
              </KeyboardAvoidingView>
            )
            : children
        }
      </View>
    </SafeAreaView>
  );
};

export default null;

ESlint will complain with the following:

error  Unused style detected: undefined.container  react-native/no-unused-styles

Maybe related to #125.

@Romick2005
Copy link

I guess you have bad styles architecture.

What is the purpose of creating two different styles?

const variants = {
  white: StyleSheet.create({
    container: {
      backgroundColor: 'white',
    },
  }),
  primary: StyleSheet.create({
    container: {
      backgroundColor: colors.primary,
    },
  }),
};

It would be better when it would be inside main styles declaration.
Or if you really want dynamic creation it can be like this:

  const viewStyle = [
    styles.container,
    {
      backgroundColor: variant === 'white' ? 'white' : colors.primary
    },
    ...logonScreen ? [styles.center] : [],
  ];

@AdamGerthel
Copy link

I'm having the same issue. Variant styles are accessed using bracket notation, but the linter doesn't understand it.

I guess you have bad styles architecture.

That's not a very helpful comment. What's bad about it?

@Romick2005
Copy link

Romick2005 commented Jan 9, 2024

I'm having the same issue. Variant styles are accessed using bracket notation, but the linter doesn't understand it.

I guess you have bad styles architecture.

That's not a very helpful comment. What's bad about it?

But I described in my comment that having multiple Styles.createStylesheet is not very convenient.

What is the point having multiple styles objects not within main style object?

@AdamGerthel
Copy link

I'm having the same issue. Variant styles are accessed using bracket notation, but the linter doesn't understand it.

I guess you have bad styles architecture.

That's not a very helpful comment. What's bad about it?

But I described in my comment that having multiple Styles.createStylesheet is not very convenient.

Not really. You said:

It would be better when it would be inside main styles declaration.

But what's "better" about it? Having an object with multiple properties, where each key is the name and each value is a style makes sense IMO when you have a lot of style changes from variant to variant. It makes it very easy to see the differences between each variant.

Let's say we have 5 variants and each variant has 3 style properties (i.e. backgroundColor, borderWidth etc). Using your suggestion that would quickly become unmanageable. Ternary operators will be super messy, and it would require at least 5 lines in the array of styles rather than just 1 line when using bracket notation.

@Romick2005
Copy link

Yeah that is your option. Could you please provide short example and I will edit it how I see it. Just to see smth

@AdamGerthel
Copy link

AdamGerthel commented Jan 9, 2024

Yeah that is your option. Could you please provide short example and I will edit it how I see it. Just to see smth

Here you go: https://snack.expo.dev/jjL9yJ3ze

@Romick2005
Copy link

Yeah that is your option. Could you please provide short example and I will edit it how I see it. Just to see smth

Here you go: https://snack.expo.dev/jjL9yJ3ze

I would go with theme for your case, but it could also be done like this: https://snack.expo.dev/@romick/variants

Pros: render code is cleaner, no array merge styles logic, all possible styles are cached and is not recreated on every render. Easy to change any style without no changes to break other variant style. All styles within createStylesheet is processed by the linter.
Cons: duplicated styles in some style object that can cause difficulties in adding some style for each variant style.

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

No branches or pull requests

3 participants