Skip to content

Conversation

@maxme
Copy link
Contributor

@maxme maxme commented Apr 19, 2018

This PR changes:

  • Toolbar styling (replaced the buttons).
  • Block content styling (added padding).
  • HTML view styling (padding + font).
  • Introduce fonts.ios.scss, fonts.android.scss that I think will be useful later to factorize some styles.

@maxme maxme requested a review from hypest April 19, 2018 07:22
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Had a first pass @maxme , left a comment.

/** @format */

.block_code {
%monospace-font {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, are there any plans to expand this class with more properties? If not, can we just introduce a constant (see variables.scss for example) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in b402c73

"config": {
"jsfiles": "index.js app/*.js app/**/*.js block-management/*.js block-management/**/*.js store/*.js store/**/*.js",
"scssfiles": "app/*.scss app/**/*.scss block-management/*.scss block-management/**/*.scss store/*.scss store/**/*.scss"
"jsfiles": "index.js src/*.js src/**/*.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving mobile-related code to src/ is worth it.

@hypest
Copy link
Contributor

hypest commented Apr 19, 2018

I think there's an issue on the iOS app with the toolbar when the block is selected:
screen shot 2018-04-19 at 15 19 59

Does that happen on your side @maxme ?

@maxme
Copy link
Contributor Author

maxme commented Apr 19, 2018

Ouch TouchableNativeFeedback is not supported on iOS... I'll have to do some changes, I'll ping you when it's ready for review.


// TODO: need to find a way to pass the include paths and the default asset files via some config
const autoImportIncludePaths = [ 'gutenberg/edit-post/assets/stylesheets' ];
const autoImportIncludePaths = [ 'src', 'gutenberg/edit-post/assets/stylesheets' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

The need to add src seems weird and I feel it's an issue withe the sass-transformer. I assume you've added this to enable the @import '../variables.scss' statement, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pushed de8629d that fixes the issue with the transformer. Follow up 0d2b501 removes the hardcoded import include path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good job, thanks

@maxme
Copy link
Contributor Author

maxme commented Apr 19, 2018

Ouch TouchableNativeFeedback is not supported on iOS... I'll have to do some changes, I'll ping you when it's ready for review.

Very disappointed about that... There are some simple solutions to have a generic button-feeling for both platform, like that:
https://medium.com/differential/better-cross-platform-react-native-components-cb8aadeba472 - but I opted for a simpler one since we're not yet at trying to make it design-ready.

Updated in 69e6717

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

🛳️it!

@hypest hypest merged commit f03087d into master Apr 19, 2018
@hypest hypest deleted the feature/better-toolbar-styling branch April 19, 2018 15:08
hypest pushed a commit that referenced this pull request Nov 2, 2018
Add placeholder properties for iOS AztecView.
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.

3 participants