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

New Wagtail Starter Page with Design #4629

Merged
merged 2 commits into from Oct 27, 2018

Conversation

Projects
None yet
7 participants
@FlipperPA
Contributor

FlipperPA commented Jun 16, 2018

This PR contains a new "Welcome to your Wagtail site" page when a user runs wagtail start [sitename]. The new page is include'd from the home_page.html template so that a new user can easily remove it when they start creating their site. It also includes translation tags in the templates so newcomers to Wagtail can see the site in their own language.

The screenshot in the tutorial of the documentation has also been updated.

Tested on and with:

  • Mac Chrome (latest)
  • Mac Firefox (latest)
  • Passes WAVE (Web Accessibility Evaluation Tool) without errors or contrast errors.

Thanks to @DanAtShenTech, @marteki, and @tomdyson for their help and suggestions.

@FlipperPA

This comment has been minimized.

Contributor

FlipperPA commented Jun 22, 2018

The Travis CI failure seems to be from Python 3.4 not installing:

3.4 is not installed; attempting download
Downloading archive: https://s3.amazonaws.com/travis-python-archives/binaries/ubuntu/14.04/x86_64/python-3.4.tar.bz2
$ curl -sSf -o python-3.4.tar.bz2 ${archive_url}
curl: (56) SSL read: error:00000000:lib(0):func(0):reason(0), errno 104
Unable to download 3.4 archive. The archive may not exist. Please consider a different version.
@gasman

This comment has been minimized.

Collaborator

gasman commented Jun 22, 2018

Travis has been pretty flaky lately. Restarted...

@thibaudcolas

👌 I've added some notes about the front-end code to improve browser support and maintainability.

margin-top: 50px;
}
}
</style>

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

Could this be defined as a compiled SCSS file instead of inline styles like this? It will be very hard to maintain in the long run if we can't have linting, autoprefixing, and other niceties like with other Wagtail code.

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

We discussed this when we redid the Django start page, and weighed the pros and cons. It was decided to do it this way (without minification and such) to keep the start page completely self-contained and easy to modify. (Reference to issues / discussions)

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

From what I can tell, Django's dev tooling for styles is much simpler than Wagtail's: they don't use a preprocessor, nor any kind of compilation (no autoprefixer), or linting. I can see why in this context introducing an additional file for a starter page wouldn't be worth it. Wagtail has all of this in place, so to me this makes it worthwhile.

What are the cons, apart from having to use the build tools while developing this?

This comment has been minimized.

@gasman

gasman Aug 7, 2018

Collaborator

@thibaudcolas I think you may be confusing the dev environment of a Wagtail project with the dev environment for developing Wagtail itself. The Wagtail project template doesn't provide any front-end build tools, so if the aim is to allow developers on a new project to modify the default template, preprocessors aren't an option.

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 8, 2018

Member

No I think I understand the distinction. Like other SCSS it would get compiled at release time, and for devs using the project template it’ll just be pointing at an external CSS file.

Is the aim to allow people to modify this? In my understanding it’s only meant as a placeholder page people are meant to delete once they start building their actual site.

<div class="column">
<div class="logo">
<a href="https://wagtail.io/" target="_blank" rel="noopener">
<svg class="figure-logo" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 342.5 126.2">

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

Could we reuse an existing SVG file instead of inlining this code?

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

I love the Wagtail Space SVG, but perhaps that branding should be kept for sprints. Should we just use the Logo available here? What are your thoughts, @tomdyson?

https://github.com/wagtail/wagtail/blob/master/wagtail/admin/static_src/wagtailadmin/images/wagtail-logo.svg

<div class="logo">
<a href="https://wagtail.io/" target="_blank" rel="noopener">
<svg class="figure-logo" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 342.5 126.2">
<title>Wagtail Logo</title>

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

This will be used as a fallback text for screen readers, so it should say something that's relevant to the link rather.

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

Fixed.

<div class="row">
<div class="column">
<div class="logo">
<a href="https://wagtail.io/" target="_blank" rel="noopener">

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

Should be rel="nofollower noopener" for browsers that do not support noopener (IE11, Edge).

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

Fixed.

html {
line-height: 1.15;
-ms-text-size-adjust: 100%;
-webkit-text-size-adjust: 100%;

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

What does this do? Can we use a standard property name instead?

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

These are ignored by most non-mobile browsers, and since we're very unlikely to be within that context, let's get rid of them! Fixed. :)

</div class="logo">
</div class="column">
<div class="column">
{% templatetag openblock %} blocktrans {% templatetag closeblock %}View the <a href="http://docs.wagtail.io/en/latest/releases/" target="_blank" rel="noopener">release notes </a>{% templatetag openblock %} endblocktrans {% templatetag closeblock %}

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

Please use an HTTPS link instead.

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

Fixed.

<p>{% templatetag openblock %} trans 'Please feel free to <a href="https://wagtail.io/slack/" target="_blank">join our community on Slack</a>, or get started with one of the links below.' {% templatetag closeblock %}</p>
</main>
<footer class="u-clearfix">
<a href="http://docs.wagtail.io/" target="_blank" rel="noopener">

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

Should be rel="nofollower noopener" for browsers that do not support noopener (IE11, Edge).

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

Please use an HTTPS link instead.

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

Fixed.

</div>
</div>
</a>
<a href="http://docs.wagtail.io/en/latest/getting_started/tutorial.html" target="_blank" rel="noopener">

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

Same here, HTTPS link 🙂, and add nofollower to the rel.

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

Fixed.

</div class="column">
</div class="row">
</header>
<main>

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

IE11 does not support the main tag, please use div role="main" instead.

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

Fixed... (mumbles incoherently about Internet Explorer for the third decade running)

</g>
</svg>
</a>
</div class="logo">

This comment has been minimized.

@thibaudcolas

thibaudcolas Jun 26, 2018

Member

There is a lot of those closing tags with attributes, could you clean this up?

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

Removed - this was from a teaching moment during the sprint. :)

</div>
<br>
<h2>{% templatetag openblock %} trans "Welcome to your new Wagtail site!" {% templatetag closeblock %}</h2>
<p>{% templatetag openblock %} trans 'Please feel free to <a href="https://wagtail.io/slack/" target="_blank">join our community on Slack</a>, or get started with one of the links below.' {% templatetag closeblock %}</p>

This comment has been minimized.

@gasman

gasman Jun 26, 2018

Collaborator

I suggest we link to https://github.com/wagtail/wagtail/wiki/Slack rather than directly to the signup page, so that people don't skip past the 'ground rules' (e.g. the existence of the #support channel).

Also, I think we should avoid target="_blank" unless there's a really good reason for it. It's not great for accessibility (people are either comfortable using tabbed browsing or not, and either way, we shouldn't force that choice onto people) and I'd like Wagtail to gently steer people away from using it - which is an easier case to make if we're minimising use of it in our own HTML.

This comment has been minimized.

@FlipperPA

FlipperPA Aug 7, 2018

Contributor

Fixed.

@thibaudcolas

Just did another pass, this time while looking at the actual page. There are a lot of styles which don't seem to be used, either missing classes in the HTML or just unused CSS. There is also a lot of layout issues because of the switch from <main> to <div role="main"> but that should be easy to fix.

Here are other specific issues:

  • At the moment there is no indication of what text is a link except for the color. On my screen it's very hard to distinguish between the #000 of the text and #246060 of the "join our community on Slack" link, so I have trouble seeing there is a link there. Could we consider always having underlines for links in text, or if not at least making the links more obvious with a sharper contrast difference compared to normal text.
  • On Firefox (v61, macOS 10.13), the lightbulb icon is not displayed.
  • In IE11, the header is too tall.
  • I think this could use a bit more love on mobile. The Wagtail Space visual seems too big, with too little whitespace above it, and too much whitespace between the bottom links. I know it might not seem important for a starter page, but if people deploy their sites with it then it'll be viewed on many more devices than what they use for development. An example of this happening in practice is Django Girls – we always make attendees deploy their sites first without any content, and they are all thrilled to look at their freshly deployed website from their phones, showing the Django starter page.
margin-top: 50px;
}
}
</style>

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

From what I can tell, Django's dev tooling for styles is much simpler than Wagtail's: they don't use a preprocessor, nor any kind of compilation (no autoprefixer), or linting. I can see why in this context introducing an additional file for a starter page wouldn't be worth it. Wagtail has all of this in place, so to me this makes it worthwhile.

What are the cons, apart from having to use the build tools while developing this?

{% templatetag openblock %} load i18n {% templatetag closeblock %}
<link rel="stylesheet" type="text/css" href="/static/admin/css/fonts.css">
<style type="text/css">
body, main {

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

I see main is still used here and in a few other places – this should be updated to use a class name now that the page uses div[role="main"]. As a general rule of thumb, I'd suggest to never write layout or component styles on bare elements (main, main p below) – this makes it harder to keep styles and HTML in sync over time.

This comment has been minimized.

@Scotchester

Scotchester Oct 24, 2018

Contributor

I reverted the change to <div role="main">. All that was needed to support IE11 is to set display: block; on main, which I did in 959d41b.

body, main {
margin: 0 auto;
}
.body, .tip {

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

As far as I can see neither of those two classes are present in the HTML.

This comment has been minimized.

@Scotchester

Scotchester Oct 24, 2018

Contributor

Removed in 959d41b.

Show resolved Hide resolved wagtail/project_template/home/templates/home/welcome_page.html Outdated
}
img {
border-style: none;
}

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

There is no img on this page.

This comment has been minimized.

@Scotchester

Scotchester Oct 24, 2018

Contributor

Removed in 959d41b.

.two {
margin-left: 0px;
-webkit-transform: none;
transform: none;

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

Those styles don't seem necessary.

This comment has been minimized.

@Scotchester

Scotchester Oct 24, 2018

Contributor

Removed in 959d41b.

<h2>{% templatetag openblock %} trans "Welcome to your new Wagtail site!" {% templatetag closeblock %}</h2>
<p>{% templatetag openblock %} trans 'Please feel free to <a href="https://github.com/wagtail/wagtail/wiki/Slack" target="_blank">join our community on Slack</a>, or get started with one of the links below.' {% templatetag closeblock %}</p>
</div>
<footer class="u-clearfix">

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

AFAIK this class doesn't exist anywhere.

This comment has been minimized.

@Scotchester

Scotchester Oct 24, 2018

Contributor

Removed in 959d41b.

</g>
</svg>
</div>
<br>

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

Shouldn't use br for vertical spacing.

This comment has been minimized.

@Scotchester

Scotchester Oct 24, 2018

Contributor

Removed in 959d41b.

<footer class="u-clearfix">
<a href="https://docs.wagtail.io/" target="_blank" rel="nofollower noopener">
<div class="option one">
<svg viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

This, and all of the other link icons, should be aria-hidden="true".

This comment has been minimized.

@Scotchester

Scotchester Oct 24, 2018

Contributor

Fixed in 959d41b.

</div>
</div>
</a>
<a href="https://docs.wagtail.io/en/latest/getting_started/tutorial.html" target="_blank" rel="nofollower noopener">

This comment has been minimized.

@thibaudcolas

thibaudcolas Aug 7, 2018

Member

For this and the release notes link – would it be possible to point to the docs for the version of Wagtail that was used?

This comment has been minimized.

@Scotchester

Scotchester Oct 24, 2018

Contributor

I added this in a way that will work unless you're using a prerelease version of Wagtail (I only noticed because I happened to still be running 2.2a0 in my local environment). If anyone has any ideas on how to handle it when {% wagtail_version %} returns a prerelease version, let me know.

@FlipperPA

This comment has been minimized.

Contributor

FlipperPA commented Oct 24, 2018

Howdy @gasman and @thibaudcolas, I talked to @tomdyson about this at DjangoCon US. This is the same sort of welcome page I contributed to Django, so I followed the methods we used there; we chose to keep the assets included in the single page design, since it will typically only be displayed once per installation, with the assets not being used afterward (as mentioned above).

I'm wondering if further changes need to happen, or if we can merge this is as it is an improvement and refine moving forward? What are your thoughts? Tagging @Scotchester who's involved as well.

@Scotchester

This comment has been minimized.

Contributor

Scotchester commented Oct 24, 2018

@Scotchester

This comment has been minimized.

Contributor

Scotchester commented Oct 24, 2018

Pushed my changes and responded to @thibaudcolas's comments.

The one thing I didn't do was try to pull out the CSS into a Sass file. I'm not opposed to it (though I think @FlipperPA's position is equally valid), but I'm not familiar enough with Wagtail's Gulp setup to feel comfortable adding a new, specialy Sass file into the mix.

@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented Oct 27, 2018

@FlipperPA @Scotchester sweet! I'll have another look now.

-webkit-transform-style: preserve-3d;
transform-style: preserve-3d;
border-top: 1px solid #efefef;
}

This comment has been minimized.

@thibaudcolas

thibaudcolas Oct 27, 2018

Member

👌 looks good to me!

</div>
</main>
<footer class="footer">
<a class="option option-one" href="https://docs.wagtail.io/en/latest/{% templatetag openblock %} wagtail_version {% templatetag closeblock %}" rel="nofollower noopener">

This comment has been minimized.

@thibaudcolas

thibaudcolas Oct 27, 2018

Member

This and the tutorial link below are missing the v for the version number in the URL path. I'll add it.

<header class="header">
<div class="logo">
<a href="https://wagtail.io/" rel="nofollower noopener">

This comment has been minimized.

@thibaudcolas

thibaudcolas Oct 27, 2018

Member

Since we removed the target="_blank", we don't need rel="nofollower noopener". I'll remove it.

@thibaudcolas thibaudcolas force-pushed the FlipperPA:feature/new-welcome-page branch from 959d41b to 4b0f0ce Oct 27, 2018

New version

@thibaudcolas

thibaudcolas approved these changes Oct 27, 2018 edited

I just pushed some commits to address what I spotted while reviewing, and move the styles to a separate file. I didn't move them to SCSS because there is no simple way to do this right now without also having the SCSS source file within the template, visible to end users. If anyone wants to see what that would be like, it's over at thibaudcolas#1.

I also made a quick video of how I tested this with VoiceOver for educational purposes. It's at https://www.youtube.com/watch?v=XxGIknNx7_o.

Tested in:

  • Mobile Safari iOS Phone 12
  • Mobile Safari iOS Phone 11
  • Chrome Android 8
  • IE Desktop 11
  • Chrome Desktop latest
  • MS Edge Desktop latest
  • Firefox Desktop latest
  • Firefox ESR Desktop 60
  • Safari macOS 11

@thibaudcolas thibaudcolas force-pushed the FlipperPA:feature/new-welcome-page branch from 1f88a28 to e03b33c Oct 27, 2018

thibaudcolas added a commit to FlipperPA/wagtail that referenced this pull request Oct 27, 2018

@thibaudcolas thibaudcolas force-pushed the FlipperPA:feature/new-welcome-page branch from e03b33c to 7844036 Oct 27, 2018

@thibaudcolas thibaudcolas merged commit f1148e7 into wagtail:master Oct 27, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/circleci: backend Your tests passed on CircleCI!
Details
ci/circleci: frontend Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 70a9a5f...7844036
Details
codecov/project 98.58% remains the same compared to 70a9a5f
Details
@Scotchester

This comment has been minimized.

Contributor

Scotchester commented Oct 27, 2018

Thanks so much for finishing it off, @thibaudcolas!

@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented Oct 27, 2018

🎉 it will be released in v2.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment