Skip to content

Conversation

@Faisal-Manzer
Copy link

@Faisal-Manzer Faisal-Manzer commented Jun 27, 2020

Normalizing it as base tailwindcss

TERMS:

THIS IS NOT A BACKWARDS COMPATIBLE PR

Issues

The main issue solved:
This is a great package with a great API interface. But it goes somewhere in some different paths with base.

  • height, width, maxHeight, minHeight, maxWidth, minWidth: These were having many intermediate values which are not in base. Issue Types for custom configs // Tailwind philosophy #18 talks about it too.
  • padding, margin: Understating with example. Currently px-10 is been converted to pX10 which should actually be converted to px10 simply.

The basic idea should be I someone reads tailwindcss documentation, he/she should good without looking deep into react-native-tailwindcss documentation

Fixes

As mentioned above the issues have been resolved. Ther are only spacing values which are present in the base. Now padding and margin are translating as referred above. i.e. px-10 to px10 and so on...

Additions

As this PR makes it somewhere even with the base. I also added a functional utility converter. (defined in utils/converter.js).

/**
 * Convert class names to the style object
 *
 * 'px-10 -mt-20 h-1/2'
 * is equivalent to [t.px10, t._mt20, h1_2]
 *
 * Returns an object instead of an array
 * Later styles can override previous and that's what you expect
 * 'px-10 px-16' is equal to 'px-16'
*/

The util is a function which can be used as

import { c, t } from 'react-native-tailwindcss';

// both are same
<View style={c('px-10 -mt-20 w-1/2')} />
<View style={ [t.px10, t._mt20, w1_2]} />

Issues with this PR

  • Documentation not updated
  • Typescript declaration not updated
  • No test written test for converter

I will try to solve this issue later.

Apology

I know package maintainers do very hard work of creating a great package and someone comes and try to do things in their own way. There is much thought in maintainers about how this should move.
Also so I needed these change immediately so I published my version (at https://github.com/Faisal-Manzer/react-native-tailwindcss/packages/289166). I will delete my version as soon as this PR gets merged.
I am very hopeful for this package.

Removed extra spacing values. Using only what is used in base tailwindcss.
Migrate Padding and Margin variants to match base tailwindcss.
px-10 should be traslated to px10 rather than pX10.

THIS IS NOT BACKWARDS COMPATIBLE.
@Faisal-Manzer
Copy link
Author

Hi, @TVke can get comments on this PR.

Added some tests for generators (more should be added)
FIXED cache issue in converter
Updated index.d.ts for generator (Not updated type declarations fully)
Updated docs to match padding and spacing
@Faisal-Manzer
Copy link
Author

Worked to be done

  • Updating type declaration
  • More examples in the readme for converter
  • Adding more tests for converter

@Faisal-Manzer
Copy link
Author

Made margin and padding backward compatible but there should a warning in README about its depreciation and should be fully removed after some release. Although changes in spacing values itself make this invalid in terms of bakwards compatiblity.

@TVke
Copy link
Owner

TVke commented Jun 27, 2020

Hi @Faisal-Manzer

thank you for the hard work.
I love your thinking about the way to release ‘2.0’.
I love de CSS parsing a lot from a user standpoint.
The only thing that bothers me a little bit is the deprecation.

I want to take the time to thoroughly check everything but for now it looks really nice.
I will leave some comments in the code lines where I have additional questions.

Greetings
Thomas

@Faisal-Manzer Faisal-Manzer requested a review from TVke July 1, 2020 03:32
Copy link
Owner

@TVke TVke left a comment

Choose a reason for hiding this comment

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

I am waiting to merge this as there are some task to be done.
You commented:

Work to be done

  - Updating type declaration
  - More examples in the readme for converter
  - Adding more tests for converter

@Faisal-Manzer
Copy link
Author

🧐 Hmmm... You need custom configs

import { Tailwind } from 'react-native-tailwindcss/tailwind';

const tailwind = new Tailwind(yourCustomConfig);
const t = tailwind.style;

<View style={[t.absolute, t.inset0, t.p4, t.bgBlue500, t._mx2, t.w1_2]} />

// OR
const c = tailwind.converter;
<View style={c('absolute inset-0 p-4 bg-blue-500 -mx-2 w-1/2')} />

Using in Hooks

import {View} from 'react-native';
import {useDarkMode} from 'react-native-dynamic'
import { Tailwind } from 'react-native-tailwindcss/tailwind';

const useTailwindCss = () => {
    const isDarkMode = useDarkMode();

    const tailwind = new Tailwind({
        theme: {
            extend: {
                colors: {
                    primary: isDarkMode? '#FFFFFF' : '#000000'
                }
            }
        }
    });
    
    return tailwind.converter;
};

const MyComponent = () => {
    const c = useTailwindCss();
    
    return <View style={c('absolute inset-0 p-4 bg-blue-500 -mx-2 w-1/2')} />

};

NOTE: All other things remains same

Except:

  • When importing theme from index then it will refer to configure theme rather that tailwind's style.

@Faisal-Manzer
Copy link
Author

This PR is getting larger but is now a step more towards actual tailwind's documentation. I have added plugin support too. Plugin support is documented here https://tailwindcss.com/docs/plugins/#app.
There are more lacking custom config support like custom separators.

I suggest to merge this and then help me with issues. One major issue is with updating types which I completely fail too. I have worked with typescript but I am not able to add custom '.d.ts' file type.

I will surely mention all issue and things there is lacking.

@Faisal-Manzer
Copy link
Author

I partially resolved #18 . Support for custom configuration is now added too. But type generator tools remain undone.

@Faisal-Manzer Faisal-Manzer requested a review from TVke July 18, 2020 20:28
Copy link
Owner

@TVke TVke left a comment

Choose a reason for hiding this comment

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

Looks good I just want a little more explanation with some parts 😊


test('alignContent', () => {
expect(alignContent).toEqual(require('./fixtures/outputs/tailwind/alignContent'));
expect(alignContent({theme, colors})).toEqual(require('./fixtures/outputs/tailwind/alignContent'));
Copy link
Owner

Choose a reason for hiding this comment

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

alignContent does not need colors or am I missing something 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Actually to save time I passed theme and colors in all closures, because there were lot of closures. I used the fact that javascript will discard any arguments passed if it is not in use and will not give error or warning.

Copy link
Owner

Choose a reason for hiding this comment

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

I understand that it easier to just pass them all but I think it is way cleaner to think about what it needs so I would love this to be only the necessary parameters.
I will do this if you don't find the time.

greetings
Thomas

Copy link
Author

Choose a reason for hiding this comment

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

I will do this.

@TVke
Copy link
Owner

TVke commented Jul 28, 2020

@Faisal-Manzer
I was wondering I want to maken this 1.2.0 but I want it backwards compatible what do you think about that?
e.g.: I was thinking in the line of keeping mT4 and mt4.

greetings
Thomas

@Faisal-Manzer
Copy link
Author

At some point of time you will release 2.0 with some breaking changes. When you will merge this PR there is a lot of changes in concept of how to use this package. The developers needs to learn the new way using this package although the code don't change.

Also, now theme exported from index.js is no longer referring to style, it is referring to theme configuration. This is a big change which can lead to refactoring for some people in many many files.

@davidgovea
Copy link
Contributor

This is cool! I want to take a deeper look at the plugin capability, especially.

The ability to do runtime changes (like for dark mode) is interesting, but it seems like it might overstep tailwindcss' responsibilities? Don't think the web version has that.. Not sure it belongs in this lib.
I noticed that tw does have a dark: media-query prefix (https://tailwindcss.com/docs/breakpoints#dark-mode). We may be able to side-step the limitations of conflicting media queries too.. I'll think more about this in #49
Could be nice to do something like

<View style={[t.bgPrimary, t.dark(t.bgGrey100)]} />

I like the capitalization changes, too, but tailwind is very careful with deprecations & breaking changes. Wonder if there's a way we can include both with deprecation warnings on the old syntax.
If not that, then at least a codemod to auto-refactor existing code would be good. I can help there

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.

4 participants