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

Minor tweaks to the welcome page #33510

Merged
merged 2 commits into from Sep 13, 2019

Conversation

@yceruto
Copy link
Member

commented Sep 9, 2019

Q A
Branch? 4.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #33189 (comment)
License MIT
Doc PR -

/cc @fabpot

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

It's better, thank you.

Some more comments:

  • On Safari, there is no margin at the bottom

image

  • The path is now correct but it looks like it's not aligned/centered (Safari and Firefox)

image

  • On my screen, the warning about the fact that this page is displayed because there is no controllers yet is not displayed. I'm wondering if we should not move it just after "Your application is now ready and you can start working on it.". And/or perhaps makes the design a bit more compact vertically.

@yceruto yceruto force-pushed the yceruto:welcome_page branch from 55f66e7 to eacb97a Sep 9, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Status: Needs Work

@yceruto yceruto force-pushed the yceruto:welcome_page branch 2 times, most recently from f5ec6fc to ddffc97 Sep 9, 2019

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

I'm testing this in Safari, Chrome and Firefox. I still cannot make it work with Firefox, but I'll keep trying to fix this:

browser-comparison

@michaelperrin

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

@javiereguiluz @yceruto First, thanks for this new project page design, it's awesome! Concerning the alignment issues, I could solve them for all browsers quite easily using Flex (not talking about Symfony's one 😄). Is there any reason you didn't use these CSS properties? It also makes CSS code cleaner in my opinion. I would be happy to share it if you wish.

Please note that I didn't test it on IE11 (if we really need this...), but that should be doable to make it work anyway if it doesn't already.

@stof

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

I don't think supporting IE 11 for this page is needed. Hopefully web developers don't use it as their main browser.
We already stopped supporting for the web profiler UI IIRC (keeping IE support for the web debug toolbar, as this one gets injected in the project UI, which may have a need to support IE)

@michaelperrin

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

Yeah I totally agree as we are targeting developers. I will have a quick look just by curiosity.

UPDATE: removed my diff post as I was comparing to the current 4.4 branch, and not this PR branch.
UPDATE 2: the current changes in this PR works well on Firefox on my side. So I am not sure that Flex is necessary.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@michaelperrin I can't see it correctly with Firefox. Look at how different is the vertical align of the monospaced text.

Firefox:
firefox

Chrome:
chrome


The problem is that both browsers are using different fonts ... because we don't define the font family explicitly, so 'monospace' is used.

I could solve the problem by setting the font family explicitly. I used the same font stack as Bootstrap's monospace:

SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace
@michaelperrin

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

You're right. My CSS flex solution wouldn't help for this. I checked with the Bootstrap's monospace font family and it makes the correctly aligned indeed.

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@javiereguiluz Let's use the same font everywhere as done with Bootstrap. I don't see any drawback.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

I've committed some changes. I share some screenshots using Firefox, which was the browser where we had some issues ... and now it looks the same as Chrome and Safari:

Desktop

desktop

Smartphone

smartphone

@fabpot
fabpot approved these changes Sep 13, 2019
@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Thank you @yceruto.

fabpot added a commit that referenced this pull request Sep 13, 2019
bug #33510 Minor tweaks to the welcome page (yceruto, javiereguiluz)
This PR was merged into the 4.4 branch.

Discussion
----------

Minor tweaks to the welcome page

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #33189 (comment)
| License       | MIT
| Doc PR        | -

/cc @fabpot

Commits
-------

acd5061 Some styling tweaks
ddffc97 minor tweaks to the welcome page

@fabpot fabpot merged commit acd5061 into symfony:4.4 Sep 13, 2019

0 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@yceruto yceruto deleted the yceruto:welcome_page branch Sep 13, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2019

Thank you @javiereguiluz ❤️

fabpot added a commit that referenced this pull request Sep 17, 2019
minor #33613 Minor updates in the new Welcome page (javiereguiluz)
This PR was merged into the 4.4 branch.

Discussion
----------

Minor updates in the new Welcome page

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

After #33510 was merged, there were some concerns about the warning message in the "Welcome Page". It's too low on the page, so it's easy to oversee it.

So, in this PR we propose to make the warning message way more visible and other minor tweaks. This is how it'd look on a smartphone and on a desktop:

![welcome-page](https://user-images.githubusercontent.com/73419/65033926-4af4b500-d946-11e9-9955-a4da60a65762.png)

Commits
-------

4517319 Minor updates in the new Welcome page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.