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

Visual refinements for native nextpage #14826

Merged
merged 4 commits into from
Apr 9, 2019
Merged

Conversation

koke
Copy link
Contributor

@koke koke commented Apr 4, 2019

Implements the easy changes requested in wordpress-mobile/gutenberg-mobile#618

nexpage-refinements

Having a dashed line is not currently supported by react-native-hr, so we'll keep investigation options as part of making this a component (#14175)

gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#819

@koke koke added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Apr 4, 2019
@koke koke requested a review from pinarol April 4, 2019 17:53
@pinarol
Copy link
Contributor

pinarol commented Apr 4, 2019

hey @koke this looks pretty good, I changed the font as System and it looked same as designs for iOS(since system font is actually San Francisco), maybe it is another quick win also.

.block-library-nextpage__text {
	color: $gray;
	text-decoration-style: solid;
	font-family: 'System'; <<<<<<<<
	text-transform: uppercase;
}

Screen Shot 2019-04-04 at 23 01 25

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

LGTM! Added a comment about system font that's all(although not feeling very strong about that)

@koke
Copy link
Contributor Author

koke commented Apr 4, 2019

Oh, I didn't know about System, it works great. However, the CSS linter is complaining about the syntax, and I'm not sure what to do about it. Do you know if we can disable it for .native.scss files or have a different set of rules? I feel like I've seen exceptions in the past, but I can't remember a specific one.

@pinarol
Copy link
Contributor

pinarol commented Apr 5, 2019

Do you know if we can disable it for .native.scss files or have a different set of rules?

Unfortunately I don't know, but I am curious now, let me dig that a bit 🤔

@hypest
Copy link
Contributor

hypest commented Apr 5, 2019

If we need a special font family for the NextPage block, how about defining a variable for it in the _native.android.scss and _native.ios.scss files that live in the gutenberg-mobile repo? I think that way all linters are happy.

@koke
Copy link
Contributor Author

koke commented Apr 9, 2019

In the end, I moved setting the font to JS. I'm turning this into a component soon, and having part of it outside the Gutenberg repo doesn't seem a good solution. We should explore what's the right approach for CSS font variables across platforms, but not on this PR.

@koke koke merged commit 27fe0a2 into master Apr 9, 2019
@koke koke deleted the rnmobile/618-nextpage-improvements branch April 9, 2019 10:16
@hypest
Copy link
Contributor

hypest commented Apr 9, 2019

I moved setting the font to JS

Can you open a ticket for this one @koke so we track it?

Having to set a font family via JS is counter-intuitive at the moment since it doesn't follow the rest of the code (setting styles via SCSS) so, it will be best if we track it.

@koke
Copy link
Contributor Author

koke commented Apr 16, 2019

Logged wordpress-mobile/gutenberg-mobile#875

This was referenced Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants