Skip to content

Minor tweaks to the welcome page #33510

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

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Minor tweaks to the welcome page #33510

merged 2 commits into from
Sep 13, 2019

Conversation

yceruto
Copy link
Member

@yceruto yceruto 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
Copy link
Member

fabpot 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
Copy link
Member Author

yceruto commented Sep 9, 2019

Status: Needs Work

@yceruto yceruto force-pushed the welcome_page branch 2 times, most recently from f5ec6fc to ddffc97 Compare September 9, 2019 13:49
@javiereguiluz
Copy link
Member

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
Copy link
Contributor

michaelperrin 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
Copy link
Member

stof 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
Copy link
Contributor

michaelperrin 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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

fabpot commented Sep 10, 2019

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

@javiereguiluz
Copy link
Member

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
Copy link
Member

fabpot commented Sep 13, 2019

Thank you @yceruto.

fabpot added a commit that referenced this pull request Sep 13, 2019
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
@yceruto yceruto deleted the welcome_page branch September 13, 2019 16:00
@yceruto
Copy link
Member Author

yceruto commented Sep 13, 2019

Thank you @javiereguiluz ❤️

fabpot added a commit that referenced this pull request Sep 17, 2019
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
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants