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

Block placeholder fixes #616

Merged
merged 17 commits into from
Feb 20, 2019
Merged

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Feb 19, 2019

Refs: #592, #549, #547

Updates the GB reference to have the placeholder fixes.
It also makes the title placeholder text o be consistent with the web and localized.

Related GB PR: WordPress/gutenberg#13945

simulator screen shot - iphone xr - 2019-02-19 at 12 44 11

@koke
Copy link
Member

koke commented Feb 19, 2019

I haven't tested yet, but from your screenshot, the placeholder doesn't look left-aligned with the title

@koke
Copy link
Member

koke commented Feb 19, 2019

Also cc @Tug for i18n string changes

@elibud
Copy link

elibud commented Feb 19, 2019

Could we avoid saying "Welcome to Gutenberg", I don't think that would make sense to most of our users. Honestly I would stick with a neutral placeholder like "Add a title".

@iamthomasbishop
Copy link
Contributor

iamthomasbishop commented Feb 19, 2019

@elibud – I think (iirc) the placeholder label text on the title does say Add a Title, unless that has changed. From what I gather, the above example shows a filled title.

One other thing that stands out to me is the margins of blocks on the initial canvas. The title feels too close to the navbar – maybe the title block is a little too tall, but it's hard to tell without seeing those boundaries.

For now, can we add a slight bit of space at the top of the canvas (above the title block) so it feels a little more balanced? Maybe 16pt?

screen shot 2019-02-19 at 9 13 05 am

@elibud
Copy link

elibud commented Feb 19, 2019

@iamthomasbishop @SergioEstevao yes, looking at the code I can see we are keeping the "Add a Title" label, please ignore me 🤦‍♀️

@SergioEstevao
Copy link
Contributor Author

@iamthomasbishop here is a new screenshot with the new margins and some background color to make it easier to figure margins.

simulator screen shot - iphone xr - 2019-02-19 at 17 30 43

@koke
Copy link
Member

koke commented Feb 19, 2019

Keep in mind that those screens are for the example app, we should make sure that the margins look right on the WordPress apps as well

@iamthomasbishop
Copy link
Contributor

@SergioEstevao Thanks for the reference. Spacing looks really wonky there – is the title vertically aligned top? That's probably why it looked like a lot of empty space 😄

In terms of spacing at the top of the canvas, I mocked this up just to get a visual reference, and 16pt/dp feels about right. It looks like this:

image

Left: title focused. Right: static.

I've also uploaded these to the Zeplin project for reference.

@SergioEstevao
Copy link
Contributor Author

@iamthomasbishop the component we use as the base for Aztec that is being used in the title always aligns the text on the top. I changed the dimensions of the component to make it more centred it now
looks like this:
simulator screen shot - iphone xr - 2019-02-19 at 21 58 04

@iamthomasbishop
Copy link
Contributor

That looks a bit better already. Might still be worth adding 16pt of margin to the top of the canvas, but this at least looks cleaner.

Side note, and maybe this is something that can be addressed in a separate issue – I noticed the color on all of the placeholder text labels is different from the preferred $gray aka #87a6bc – can we match those? Thank you!

@SergioEstevao
Copy link
Contributor Author

@iamthomasbishop updated the colors:

simulator screen shot - iphone xr - 2019-02-20 at 00 21 32

@SergioEstevao
Copy link
Contributor Author

@daniloercoli do you mind have a look in Android to see if things look ok after these changes?

@daniloercoli
Copy link
Contributor

daniloercoli commented Feb 20, 2019

It does seem to work fine on Android!
(can't delete the title as reported in slack convo, that's a problem not related to this PR, but otherwise this is ok)

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Code looks good and the paragraph placeholders looks good and aligned with the title placeholder. 🎉
Tested in both WPiOS and WPAndroid ✅

I did notice something regarding the default block appender: It seems to be styled with a different color, and on iOS it has a different vertical alignment compared with the paragraph placeholder:

placeholders

I'm not sure if the colors difference is intended and/or how important are those details right now.
Let me know what do you think and cc to @iamthomasbishop

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

@SergioEstevao - The colors issue I mentioned was my bad! Colors are good ✅

After those new commits, the vertical alignment looks a lot better. ✨

@iamthomasbishop
Copy link
Contributor

Thanks for all of the quick iterations/feedback, @SergioEstevao and @etoledom! When ready, can you share screenshots of the current state on iOS/Android for design review? TY!

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.

6 participants