Feature #9314

Feature #8588: Assistant: Get ready for a first web implementation

Feature #9214: Assistant: Decide web technology for the web assistant

Assistant: Fix our ikiwiki CSS to not break when bootstrap is added on top of if

Added by sajolida about 2 years ago. Updated over 1 year ago.

Status:ResolvedStart date:05/01/2015
Priority:ElevatedDue date:
Assignee:-% Done:

100%

Category:Infrastructure
Target version:Tails_1.8
QA Check: Blueprint:
Feature Branch:web/9314-fix-ikiwiki-CSS Easy:
Type of work:Website Affected tool:Installation Assistant

Description

Our idea for the Installation Assistant is to add bootstrap.css on top of our custom ikiwiki style. This works 95% but some stuff has to be adjusted in our current CSS code.

0001-Add-bug-about-default-behaviour-of-meta-plugin-for-s.patch Magnifier (2.24 KB) sajolida, 05/28/2015 06:33 PM

no-background.png (68.5 KB) sajolida, 09/02/2015 09:15 AM

Associated revisions

Revision 4ba3d397
Added by sajolida over 1 year ago

Merge remote-tracking branch 'origin/web/9314-fix-ikiwiki-CSS' into web/assistant (Closes: #9314)

History

#1 Updated by sajolida about 2 years ago

  • Parent task deleted (#9313)

#2 Updated by sajolida about 2 years ago

  • Parent task set to #9214

Actually, this should be done as part of the decision on which technology to use for the web assistant, because if it doesn't work we should decide again what to do.

#3 Updated by sajolida about 2 years ago

  • Affected tool set to Installation Assistant

#4 Updated by sajolida about 2 years ago

  • Description updated (diff)

#5 Updated by tchou about 2 years ago

  • Assignee changed from tchou to sajolida

I've tried som stuffs. I think it would be better to have in the <head> bootstrap.css then style.css then local.css then maybe a router.css . I don't know if it's possible because of the [[!meta stylesheet="bootstrap" rel="stylesheet"]] in mdwn page that includes bootstrap on specific pages.

Anyway I tried to find a workaround and an other way to do, by adding an other [[!meta stylesheet="router" rel="stylesheet"]] in the pages, just to overide bootstrap.css. For a raison I can't understand it seems that the title="bootstrap" attribute that ikiwiki add make impossible to have two stylesheets. But, if we add a blank title attribute then it's ok.

I had a look to the w3c spec. I'm not used to read this kind of documents, but attribute title in <link> seems ok (http://www.w3.org/TR/html5/document-metadata.html#the-link-element then http://www.w3.org/TR/html5/dom.html#global-attributes and http://www.w3.org/TR/html5/dom.html#attr-title), so I don't understand why I get this bug (I'm using Chromium [edit : same behavior with Iceweasel.] ).

Finaly, with this code, it's possible to have bootstrap and overide it :

[[!meta stylesheet="bootstrap" rel="stylesheet" title=""]]
[[!meta stylesheet="router" rel="stylesheet" title=""]]

#6 Updated by tchou about 2 years ago

As fas as I understand, when there is a title in a <link>, it become a kind a prefered link. But, it's possible to have group of prefered links. So this code may be the good way :

[[!meta stylesheet="bootstrap" rel="stylesheet" title="bootstrap"]]
[[!meta stylesheet="router" rel="stylesheet" title="bootstrap"]]

I feel that ikiwiki should not add the title attribute par default, he should add it when it's explict (but there is probably a good raison for that).

#7 Updated by tchou about 2 years ago

Note thant I won't go further, because I need to know which file comes after the other.

#8 Updated by tchou about 2 years ago

  • QA Check set to Info Needed

#9 Updated by sajolida about 2 years ago

I think it would be better to have in the <head> bootstrap.css then style.css then local.css then maybe a router.css.

All the meta directives are added to the main template in a single block. If you look at page/template.tmpl this corresponds to:

<TMPL_IF META><TMPL_VAR META></TMPL_IF>

If that's really needed we can move this above the other stylesheets. But I don't think we can split this block in two and have bootstrap, style, local, and then router.

By moving this line, I managed to do bootstrap, then router, then style and it indeed looks very different.

Finaly, with this code, it's possible to have bootstrap and overide it :

!meta stylesheet="bootstrap" rel="stylesheet" title=""
!meta stylesheet="router" rel="stylesheet" title=""

I agree with your conclusion and I think that we should use this code for now.

I feel that ikiwiki should not add the title attribute par default, he should
add it when it's explict (but there is probably a good raison for that).

I think that's a bug in ikiwiki. Nothing in the source code explains this decision and I think this should not be the default. I wrote a bug report but couldn't post it because I was treated as spam... Me?! :)

I'll try to forward it through intrigeri who has high creds there. For the time being find it in attachment.

#11 Updated by sajolida almost 2 years ago

  • Priority changed from Normal to Elevated

No progress in 3 months, raising priority.

#12 Updated by sajolida almost 2 years ago

Ah, and here is some useful background information for when you'll be working on this:

https://www.youtube.com/watch?v=UXjUQnSi9zE

:)

#13 Updated by tchou over 1 year ago

  • Assignee changed from tchou to sajolida
  • QA Check set to Ready for QA

I fixed the width in 7f2c914224681e2e6a3441ac83147fec6d4bf0de (https://git-tails.immerda.ch/tchou/tails/)

I think the current website and the assistant with bootstrap are close enough now.

sajolida, any other issues ? (and thank you, your trick was very usefull)

#14 Updated by sajolida over 1 year ago

  • File no-background.png added
  • Assignee changed from sajolida to tchou
  • QA Check changed from Ready for QA to Info Needed

Here I don't see the background image on the version with bootstrap. See screenshot in attachment. Can you fix this?

I guess you decided to remove the sidebar to put emphasis on the fact that the assistant was a different workflow than the rest of the website, still, in the current version we're taking all the horizontal space of the screen. I expect this to be problematic on the instructions for example where very long lines are harder to read than shorter ones. Have the screen split horizontally in different blocks allows for the main content to have shorted and more readibly lines.

#15 Updated by sajolida over 1 year ago

  • Target version set to Tails_1.7

This needs to be ready for the testing session in November.

#16 Updated by tchou over 1 year ago

  • Assignee changed from tchou to sajolida
  • QA Check changed from Info Needed to Ready for QA
  • Feature Branch set to web/9314-fix-ikiwiki-CSS

Say me if it's ok. I will add the :

[[!meta stylesheet="bootstrap" rel="stylesheet" title=""]]
[[!meta stylesheet="assistant-layout" rel="stylesheet" title=""]]

step by step after on all the pages (first in the router)

#17 Updated by sajolida over 1 year ago

  • Assignee changed from sajolida to tchou
  • QA Check changed from Ready for QA to Info Needed

Thanks for working on a topic branch!

Yes, you should add [[!meta stylesheet="something" rel="stylesheet" title=""]] wherever needed on the pages of the assistant. But:

  • Right now we don't need bootstrap on the steps. I've already done something that works without it.
  • You're current "assistant-layout" does general formatting that will be relevant for all pages in the assistant using bootstrap. Then on, maybe you should add different modular stylesheets for the router, the download, and the overview. I'll let you decide when this become relevant.
  • What about putting these stylesheets that are for the assistant only under install/inc/stylesheets as I've done for the steps?
  • What about naming it assistant.css only?

I should have a closer look at the CSS at some point as well. But I'd like to clarify these points before considering merging it into web/assistant.

#18 Updated by tchou over 1 year ago

  • Assignee changed from tchou to sajolida

sajolida wrote:

Thanks for working on a topic branch!

Yes, you should add [[!meta stylesheet="something" rel="stylesheet" title=""]] wherever needed on the pages of the assistant. But:

  • Right now we don't need bootstrap on the steps. I've already done something that works without it.

I think we need the same layout for all pages. It will be easiest to setup the responsiveness and to maintain.

  • You're current "assistant-layout" does general formatting that will be relevant for all pages in the assistant using bootstrap. Then on, maybe you should add different modular stylesheets for the router, the download, and the overview. I'll let you decide when this become relevant.
  • What about putting these stylesheets that are for the assistant only under install/inc/stylesheets as I've done for the steps?

that's true.

  • What about naming it assistant.css only?

Why not.

I should have a closer look at the CSS at some point as well. But I'd like to clarify these points before considering merging it into web/assistant.

#19 Updated by tchou over 1 year ago

Commited name changing and move of assistant-layout.

#20 Updated by sajolida over 1 year ago

I think we need the same layout for all pages. It will be easiest to setup the responsiveness and to maintain.

All-right, then I reopened #9206 and reassigned it to you (#9206#note-24).

#21 Updated by sajolida over 1 year ago

  • QA Check changed from Info Needed to Ready for QA

You reassigned this to me but without putting it as Ready for QA though I think that was you intention. Correct me if this is not Ready for QA and clarify what "Info Needed" you expect from me.

#22 Updated by sajolida over 1 year ago

  • Assignee changed from sajolida to tchou
  • QA Check changed from Ready for QA to Dev Needed

I reviewed this branch first since it's blocking some others. It would be useful to solve this one first.

First of all, thanks you a lot for moving to topic branches. I found
the diff of this one very easy to review. It's short and the result is
pixel perfect.

Also congrats for getting rid of trailing spaces! Still I couldn't control
myself and did a bunch of syntax nitpicking on bf84140 and
cf5ffce. Please make sure that I didn't break anything and feel free
to do the same yourself next.

I also fixed a broken directive with 5003079. Maybe @git grep" could
have helped you spot that.

Make sure you merge origin/web/9314-fix-ikiwiki-CSS into your local
branch to integrate my changes.

The only blocker I see here is regarding the titling. You put @.title { display: none !important; } but then, as proven in b495fe7 it
forces you to duplicate what we had previously in
[[!meta title@ as a
<h1>. And [[!meta title cannot be skipped because that's what is
used in the <title> tag. I would really much prefer not doing this
information duplication on every page. And as I couldn't find any
comment on this in your Git history I wonder what lead you to do that
or whatelse did you try.

As a good practice for the future, I really recommend always doing
git status, git diff and git diff --cached of your changes
before committing, actually while working it's very useful as
well. It'll prevent much noise in the Git history, like "COUCOU" in
66c4e4e and other missing files :)

#23 Updated by tchou over 1 year ago

  • Assignee changed from tchou to sajolida
  • QA Check changed from Dev Needed to Info Needed

Maybe I missed something with the [[!meta title]] tag. Did I removed it ? With the .mdwn I used from https://git-tails.immerda.ch/tails/tree/wiki/src/install.mdwn?h=web/assistant I only had "install" or "os" as <h1> deducted from file names and I think they are not titles (it's "Welcome to the Tails Installation Assistant" and "Which operating system are you installing Tails from?"

Probably it's better to use [[!meta title]] tags with explicit content yes.

#24 Updated by sajolida over 1 year ago

  • Target version changed from Tails_1.7 to Tails_1.8

Tails 1.7 is out now, please don't assign ticket to me on past versions :)

#25 Updated by sajolida over 1 year ago

  • Assignee changed from sajolida to tchou

This issue around [[!meta title is a pending topic that needed clarification as we didn't settled on something clear yet. So I don't mean to putting the blame on you for doing a mistake and let's instead discuss it for real.

Some of the pages we are working on have what I would call a "title". For example, "Install from another Tails" on linux/clone.mdwn, "Download and verify" on download.mdwn, "Debian user" on debian.mdwn (though I never like this one).

The default mechanism to do this in ikiwiki is through the [[!meta title directive. This is what sets the <head> directive and what we've been using everywhere else on the website.

In your commit 19f637f you're hiding it (display: none) on every page. This means that on the pages where we have such kind of titles, if we want to have something relevant both in <head> and in <body> we'd have to duplicate that information and add it back in <body> as a <h1>. That's exactly what you're doing in b495fe7 with:

+<h1>Download and verify</h1>
+
+[[!meta title="Download and verify"]]

I don't like this kind of information duplication and want to avoid it. I think that instead we should have the original [[!meta title directive integrated in out bootstrap template somehow. Then, on the few pages where we might not want to have this title, then we should be able to specifically disable it (maybe through an additional CSS just to hide it).

Is this more clear? What do you think?

#26 Updated by tchou over 1 year ago

  • Assignee changed from tchou to sajolida
  • QA Check changed from Info Needed to Ready for QA

I removed the title hidding and will modifiy the ikiwiki in an other branch after merging.

#27 Updated by sajolida over 1 year ago

  • Status changed from Confirmed to Resolved
  • % Done changed from 0 to 100

#28 Updated by sajolida over 1 year ago

  • Assignee deleted (sajolida)
  • QA Check deleted (Ready for QA)

Also available in: Atom PDF