Style using CSS Modules #36

hypest · 2018-04-12T20:30:25Z

Start using SASS to define the styling of the views.

This solution is based on https://github.com/kristerkari/react-native-css-modules.
The idea here is to eventually be able to load the .scss files from the GB sourcetree, ideally with only some modifications/specializations.

The core/code block is styled with setting font-family to monospace. See here.
The Jest tests don't support the CSS module loading (yet, hopefully) so, mocking the CSS modules for now
On the Gutenberg code tree side, the styles are specialized by defining .native.scss versions of the SASS files

For now, only set the font to monospace.

maxme · 2018-04-16T07:55:45Z

I just noticed font: monospace is not working on iOS. I get the following error:

Unrecognized font family 'monospace'

+[RCTFont updateFont:withFamily:size:weight:style:variant:scaleMultiplier:]
    RCTFont.mm:295
-[RCTFontAttributes updateFont]
-[RCTFontAttributes setFontFamily:]

Here is a list of the stock fonts by platform. I think it should be Courrier ~~or Courrier New~~ for iOS.

maxme · 2018-04-16T07:56:11Z

Should we export the style from block-management/toolbar.js to a .scss file?

maxme · 2018-04-16T08:01:55Z

block-management/block-manager.scss

@@ -0,0 +1,20 @@
+.container {


We should maybe start adding /** @format */ to style files. And check/format them with prettier.

Good idea! Done with 686c0a3.

maxme · 2018-04-16T08:03:01Z

.babelrc

+    [
+      "react-native-platform-specific-extensions",
+      {
+        "extensions": ["native.scss", "css", "scss", "sass"]


Are ios.scss and android.scss missing from this list?

Actually, there is no need for the native.scss anyway so, removed with 74c88a6.

Platform-specific file resolution is not yet supported by the react-native-sass-transformer so, just use a common denominator. `monospace` is not supported on iO so, for now just use `courier` which is common between the platforms.

hypest · 2018-04-16T10:05:45Z

I think it should be Courrier or Courrier New for iOS.

I changed monospace to courier as the common denominator with 05b1dd8. We can't have platform-specific files yet so, just using what's common between the platforms for now.

Should we export the style from block-management/toolbar.js to a .scss file?

Done with 8d0821b for completeness.

Will work on the rest of the feedback.

Notice the code duplication between the block-holder.android.scss and block-holder.ios.scss files. This is needed because platform specific scss @import statements are not supported yet.

Since Flow+Haste can't properly resolve the platform-specific CSS Modules.

hypest · 2018-04-16T12:19:13Z

Differentiated the platform specific font-family for the Code block with 583335d. Using monospace on Android and courier on iOS. Note: the implementation is not optimal since there is code (well, CSS) duplication in the block-holder style files. That will get fixed when the SCSS @import statements start to properly resolve the platform specific file if any.

In the meantime, Flow can't properly resolve the platform specific style files as well so, need to mock the module resolution for Flow too (see dd23ef4).

hypest · 2018-04-16T12:28:11Z

I think I addressed all the feedback so far @maxme , ready for another pass, thanks!

maxme · 2018-04-16T12:59:47Z

Something weird happens on iOS:

https://bia.is/s/PQRr/style-sass-ios.mov

Note: everything looks good in the master branch.

hypest · 2018-04-16T13:44:57Z

Something weird happens on iOS:

Fixed with 590fea5 @maxme .

It seems that `flex: 1` is not interpreted correctly (flexGrow ends up 0 which causes the list to effectively collapse on iOS. For some reason, it doesn't cause a problem on Android).

maxme · 2018-04-16T15:10:32Z

!

…ncy-installer-script-2 Removes a command that no longer exists from 'yarn clean:install'.

hypest added 6 commits April 12, 2018 22:34

Initial support for loading .scss style files

01d7317

Load styles from the CSS module for block-manager

f359c38

Load native CSS module in plain-text

fcffc1d

Use a CSS module for block-holder styling

4c51133

Set the core/code styling

712860c

For now, only set the font to monospace.

Mock the CSS modules in Jest test for now

9d86bf5

hypest requested a review from maxme April 12, 2018 20:30

maxme self-assigned this Apr 16, 2018

maxme reviewed Apr 16, 2018

View reviewed changes

hypest added 2 commits April 16, 2018 13:01

Set Code block font to Courier

05b1dd8

Platform-specific file resolution is not yet supported by the react-native-sass-transformer so, just use a common denominator. `monospace` is not supported on iO so, for now just use `courier` which is common between the platforms.

Scss file for the toolbar as well

8d0821b

hypest added 3 commits April 16, 2018 14:08

No need for explicit mobile native extension

74c88a6

Platform specific font family for the code block

583335d

Notice the code duplication between the block-holder.android.scss and block-holder.ios.scss files. This is needed because platform specific scss @import statements are not supported yet.

Mock scss loading for Flow

dd23ef4

Since Flow+Haste can't properly resolve the platform-specific CSS Modules.

hypest force-pushed the feature/style-sass branch from df60449 to dd23ef4 Compare April 16, 2018 12:14

Enable Prettier for the scss files

686c0a3

Explicitly set flexBasis to auto

590fea5

It seems that `flex: 1` is not interpreted correctly (flexGrow ends up 0 which causes the list to effectively collapse on iOS. For some reason, it doesn't cause a problem on Android).

hypest force-pushed the feature/style-sass branch from ccd5038 to 590fea5 Compare April 16, 2018 13:45

Only list scss files in "scssfiles"

89d7911

maxme merged commit 92c08a9 into master Apr 16, 2018

maxme deleted the feature/style-sass branch April 16, 2018 15:10

hypest mentioned this pull request Sep 4, 2018

React Native - SVG icons for Inserter WordPress/gutenberg#9294

Merged

4 tasks

hypest pushed a commit that referenced this pull request Nov 2, 2018

Merge pull request #36 from wordpress-mobile/try/ios-carthage-depende…

c5951de

…ncy-installer-script-2 Removes a command that no longer exists from 'yarn clean:install'.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style using CSS Modules #36

Style using CSS Modules #36

hypest commented Apr 12, 2018 •

edited

Loading

maxme commented Apr 16, 2018 •

edited

Loading

maxme commented Apr 16, 2018

maxme Apr 16, 2018

hypest Apr 16, 2018

maxme Apr 16, 2018

hypest Apr 16, 2018

hypest commented Apr 16, 2018

hypest commented Apr 16, 2018

hypest commented Apr 16, 2018

maxme commented Apr 16, 2018

hypest commented Apr 16, 2018 •

edited

Loading

maxme commented Apr 16, 2018

Style using CSS Modules #36

Style using CSS Modules #36

Conversation

hypest commented Apr 12, 2018 • edited Loading

maxme commented Apr 16, 2018 • edited Loading

maxme commented Apr 16, 2018

maxme Apr 16, 2018

Choose a reason for hiding this comment

hypest Apr 16, 2018

Choose a reason for hiding this comment

maxme Apr 16, 2018

Choose a reason for hiding this comment

hypest Apr 16, 2018

Choose a reason for hiding this comment

hypest commented Apr 16, 2018

hypest commented Apr 16, 2018

hypest commented Apr 16, 2018

maxme commented Apr 16, 2018

hypest commented Apr 16, 2018 • edited Loading

maxme commented Apr 16, 2018

hypest commented Apr 12, 2018 •

edited

Loading

maxme commented Apr 16, 2018 •

edited

Loading

hypest commented Apr 16, 2018 •

edited

Loading