Fix media merge with variants and compound variants #33

tkow · 2022-10-29T10:16:36Z

Closes #34
Check after #31 , #32.

I fixed media properties can be used with variants and compound variants styles.

I removed these code, because it's been ever ignored and not worked due to key format is used instead of @key. In addition, now media properties must be kept as media variants style for work.

I add new examples of this features.

Fix/media

Feat/merge media

Temzasse · 2022-10-29T14:43:49Z

Thank you for these PRs! On a quick glance they look very good 👍🏻 I'll try to find some time to review and test them locally 🙂

It looks like these three PRs build on top of each other so I'm wondering if it would be easier for me to review just the latest PR that contains all the fixes 🤔 They are all related to responsive styles so it wouldn't be too bad to review all the fixes as one big PR.

Is it okay to you if I just review and test this PR and close the other two?

tkow · 2022-10-29T19:01:59Z

@Temzasse

Is it okay to you if I just review and test this PR and close the other two?

Yes, of course.
Thank you for reaction and taking your time.

Temzasse · 2022-10-30T10:25:32Z

Thank you for setting up the Expo Web support in the example app! 🙌🏻

I noticed that running the example app in iOS Simulator doesn't seem to be working anymore because it's not able to resolve stitches-native. I think it's very nice to be able to test the lib in a web browser but the main target of this lib is native app development so it's very important to be able to test the app inside iOS/Android simulator.

I wonder why the web version is working but not the native one 🤔

tkow · 2022-10-30T13:26:58Z

Sorry for this pitfall. It may be caused by expo and react-native latest versions.
I will check tomorrow both android and iOS simulator. Let me share if I find the reason.

Temzasse · 2022-10-30T13:42:19Z

I think we can have both metro and webpack configs in the example Expo app. Metro will be used for native and webpack for web. I took a look at this RN library called Zeego that has both config files in the example app. I will play around with the bundler setup today and see if I can get it to work.

tkow · 2022-10-30T14:02:59Z

Ah... I see. I certainly removed metro.config.js. The reason is mentioned in #31. But, you're right, we must use metro or repack as native module bundler. Especially I don't know whether we can use repack with expo so we should use metro. Thank you for investigation and I'll try to fix it too, though no problem as if you finish to work earlier than me.

Temzasse · 2022-10-30T15:08:22Z

I think I got it working with both webpack and metro. Can you give me rights to push to your PR branch?

tkow · 2022-10-30T15:17:54Z

@Temzasse Thank you, I worked just restoring metro.config.js, too. If you do another way and want more fix, could you do after merging my code?

Temzasse

This the first review round for the example app related changes.

Next I will take a look at the lib changes.

example/package.json

example/webpack.config.js

tkow · 2022-10-30T15:19:30Z

I tested and worked in our local environment like this.

Temzasse · 2022-10-30T15:19:53Z

@Temzasse Thank you, I worked just restore metro.config.js, too. If you do another way and want more fix, could you do after merging my code?

Sure that works too 👍🏻

tkow · 2022-10-30T15:31:38Z

@Temzasse
Sorry, let you review many times.
If you still have problems to work, I will set permission my repository.

Temzasse

All in all changes to the internals look very good 👍🏻

Found a few places where we can cleanup the code a bit.

I will still test the implementation a bit more in order to make sure all different cases of media styles work as expected.

Temzasse · 2022-10-30T15:29:56Z

src/internals/utils.js

@@ -103,16 +118,41 @@ export function processTheme(theme) {
  return { definition, values };
 }

+const THEME_PRIORITIZED_KEYS = [


There is no need for priority ordering for these keys - they can be in any order. This means that you can derive them from the THEME_VALUES object from constants.ts:

const THEME_KEYS = Object.keys(THEME_VALUES);

Temzasse · 2022-10-30T15:31:39Z

src/internals/utils.js

-        ...acc,
-        ...processStyles({ styles: utils[key](val), theme, config }),
-      };
+      // NOTE: Deepmerge for media propeties.


NIT: typo popeties -> properties

Temzasse · 2022-10-30T15:33:08Z

src/internals/utils.js

+  'letterSpacings',
+];
+
+function searchHaveTokenConfigKey(theme, themeMap, key) {


NIT: a better name for this function would be something like getThemeScaleKey and instead of using variable called configKey in various places in this file a more accurate name would be scaleKey.

Okey, but colors key is also included. So, I'll rename getThemeKey and themeKey.

Temzasse · 2022-10-30T15:36:19Z

src/internals/utils.js

@@ -159,6 +177,8 @@ export function processStyles({ styles, theme, config }) {
    } else if (typeof val === 'object' && val.value !== undefined) {
      // Handle cases where the value comes from the `theme` returned by `createStitches`
      acc[key] = val.value;
+    } else if (typeof acc[key] === 'object' && typeof val === 'object') {


Could you add a comment for this case so that it's easier to understand what is happening here when I or someone else comes back to this codebase after some time 😄

Temzasse · 2022-10-30T15:39:24Z

src/internals/index.js

-              Object.entries(config.media).forEach(([key, val]) => {
-                const breakpoint = `@${key}`;
-
+              if (breakpoint) {


This if could be merged with the below if to reduce the number of ifs:

if (breakpoint && propValue[breakpoint] !== undefined) {}

Temzasse · 2022-10-30T15:41:44Z

src/internals/index.js

-                  if (match) {
-                    styleSheetKey = `${prop}_${propValue[breakpoint]}`;
-                  }
+                  styleSheetKey = `${prop}_${propValue[breakpoint]}`;


This can also be simplified so that there is only one if:

if (val === true || typeof val === 'string') { styleSheetKey = `${prop}_${propValue[breakpoint]}`; }

Temzasse · 2022-10-30T15:49:55Z

@tkow Thank you for being patient with my many reviews 🙏🏻 After making the changes requested in my last review round I can merge this PR and fix anything else that comes up on my own 🙂

tkow · 2022-10-30T15:57:26Z

@Temzasse
Sure. Thank you for many advices. I can take time an hour now, fix them at least in an hour.

By the way, I don't know why, but this instruction can't be applied.(UI doesn't exist)

So, in already our repository public you may be able to push without any setting. If you can't do so and need to do, I'll suggest another way after fixing the your review points.

…(comment)

Temzasse

Awesome job! Thank you very much for this 🙌🏻 ✨

I will merge this after I have had the time to test it a bit more 🙂

tkow · 2022-10-31T07:25:25Z

Sorry, after review approved, but I found the type problem for #34 and, example's @type/react version's not compatible with installed react. Then, I add compoundVariants example for check.
These are not so much change and essentially bug fix, thus I'll merge it to PR.

See,

61d83b8

and

…compoundVariables example * fix: Temzasse#34 * chore: compound variants example

tkow · 2022-10-31T07:48:51Z

@Temzasse

In addition, I don't know why but found the bug defaultVariants combination doesn't match compoundVariants, though
the passed props work. If you want to check, please change my example.

defaultVariants: {
      heading: 'h1',
      underlined: true,
},

~~This bug may have ever been and not very serious because passing props explicitly so, could I ask you to get it out of scope in this PR?~~
I can fix it easily, so bring it to this PR. Now example looks that.

As I'm already using these new features as private git submodules. So, when I find something wrong and it's mission critical for our development, I may work for this library.

Stitches and it for the native, really most awesome library as I have ever seen especially in the point of cross platform availability while it may be hard with streaming SSR , and I want to keep using them for long. I appreciate your nice work public.

Temzasse · 2022-10-31T11:23:21Z

@tkow thank you so much for these fixes! 👏🏻

Let's not add any more fixes outside of the media styles related bug fixes or improvements into this PR so that that I'm able to review things more efficiently.

I will try to merge this as soon as possible so that you can get these fixes from the npm instead of your git submodule 🙂

tkow and others added 10 commits October 28, 2022 06:44

fix: test latest expo

fdeb077

feat: watch mode

81d1deb

fix: media can be applyied top level

58c7bc0

fix: wrong replacement

868d02f

Merge pull request #1 from MOCHI-inc-JAPAN/fix/media

9617a4a

Fix/media

feat: media object merge

78b5571

refactoring: chage if condition to function

416b875

Merge pull request #2 from MOCHI-inc-JAPAN/feat/merge-media

5933004

Feat/merge media

feat: variants utils example

6e86c2e

feat: enable variant media style

d440978

tkow force-pushed the fix/media-merge-with-variants branch from 984eac0 to d440978 Compare October 29, 2022 10:22

fix: test

561037e

fix: native example support

1f46133

Temzasse requested changes Oct 30, 2022

View reviewed changes

example/package.json Outdated Show resolved Hide resolved

example/webpack.config.js Outdated Show resolved Hide resolved

tkow added 2 commits October 31, 2022 00:25

fix: dev dependencies

d45b3d1

fix: improvement dev config

496a0bf

tkow requested a review from Temzasse October 30, 2022 15:38

Temzasse requested changes Oct 30, 2022

View reviewed changes

tkow added a commit to MOCHI-inc-JAPAN/stitches-native that referenced this pull request Oct 30, 2022

fix: Temzasse#33 (comment)

ad7f4fa

tkow added a commit to MOCHI-inc-JAPAN/stitches-native that referenced this pull request Oct 30, 2022

fix: Temzasse#33 (comment) and Temzasse#33 (comment)

e184257

tkow added a commit to MOCHI-inc-JAPAN/stitches-native that referenced this pull request Oct 30, 2022

fix: Temzasse#33 (comment)

a1c14a6

tkow added a commit to MOCHI-inc-JAPAN/stitches-native that referenced this pull request Oct 30, 2022

fix: Temzasse#33 (comment)

9d2c6c8

tkow added a commit to MOCHI-inc-JAPAN/stitches-native that referenced this pull request Oct 30, 2022

fix: Temzasse#33 (comment)

da7f5b8

tkow added 4 commits October 31, 2022 01:40

fix: Temzasse#33 (comment) and Temzasse#33 (comment) and Temzasse#33 …

e26d79e

…(comment)

fix: Temzasse#33 (comment)

100d5df

fix: Temzasse#33 (comment)

b25e369

fix: Temzasse#33 (comment)

ed1456d

tkow force-pushed the fix/media-merge-with-variants branch from da7f5b8 to ed1456d Compare October 30, 2022 16:40

tkow requested a review from Temzasse October 30, 2022 16:42

This was referenced Oct 30, 2022

Fix media style can be applied as top level property #31

Closed

Object properties can be merged instead of overwrite #32

Closed

Temzasse approved these changes Oct 30, 2022

View reviewed changes

This was referenced Oct 31, 2022

Css function Composer type extends String type require length property is not needed #34

Closed

Fix/css type #35

Closed

Fix css type exclude string's length and type error of react and add …

61d83b8

…compoundVariables example * fix: Temzasse#34 * chore: compound variants example

fix: default variants mathes compound varinats (#7)

ffcbad0

Temzasse approved these changes Oct 31, 2022

View reviewed changes

Temzasse merged commit 60f5c35 into Temzasse:main Oct 31, 2022

tkow deleted the fix/media-merge-with-variants branch November 8, 2022 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix media merge with variants and compound variants #33

Fix media merge with variants and compound variants #33

tkow commented Oct 29, 2022 •

edited

Loading

Temzasse commented Oct 29, 2022

tkow commented Oct 29, 2022

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022 •

edited

Loading

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022 •

edited

Loading

Temzasse left a comment

tkow commented Oct 30, 2022

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022

Temzasse left a comment

Temzasse Oct 30, 2022

Temzasse Oct 30, 2022

Temzasse Oct 30, 2022

tkow Oct 30, 2022

Temzasse Oct 30, 2022

tkow Oct 30, 2022

Temzasse Oct 30, 2022

Temzasse Oct 30, 2022

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022 •

edited

Loading

Temzasse left a comment

tkow commented Oct 31, 2022 •

edited

Loading

tkow commented Oct 31, 2022 •

edited

Loading

Temzasse commented Oct 31, 2022

Fix media merge with variants and compound variants #33

Fix media merge with variants and compound variants #33

Conversation

tkow commented Oct 29, 2022 • edited Loading

Temzasse commented Oct 29, 2022

tkow commented Oct 29, 2022

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022 • edited Loading

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022 • edited Loading

Temzasse left a comment

Choose a reason for hiding this comment

tkow commented Oct 30, 2022

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022

Temzasse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Temzasse commented Oct 30, 2022

tkow commented Oct 30, 2022 • edited Loading

Temzasse left a comment

Choose a reason for hiding this comment

tkow commented Oct 31, 2022 • edited Loading

tkow commented Oct 31, 2022 • edited Loading

Temzasse commented Oct 31, 2022

tkow commented Oct 29, 2022 •

edited

Loading

tkow commented Oct 30, 2022 •

edited

Loading

tkow commented Oct 30, 2022 •

edited

Loading

tkow commented Oct 30, 2022 •

edited

Loading

tkow commented Oct 31, 2022 •

edited

Loading

tkow commented Oct 31, 2022 •

edited

Loading