Skip to content
This repository has been archived by the owner on Aug 3, 2024. It is now read-only.

Introduce NewOcean theme #782

Closed
wants to merge 27 commits into from
Closed

Conversation

alexbiehl
Copy link
Member

@alexbiehl alexbiehl commented Mar 21, 2018

These are the rebased commits from #721 on the ghc-8.4 branch. The only change I made was putting them into NewOcean.std-theme to preserve the old Ocean theme.

/cc @NunoAlexandre @gbaz @hvr

@alexbiehl alexbiehl changed the title Introduce NewOcean Introduce NewOcean theme Mar 21, 2018
@alexbiehl
Copy link
Member Author

I am currently looking at the documentation for haddock-library, generated with this patch and I see the following:

bildschirmfoto 2018-03-21 um 10 00 30

The Source should be aligned at the right end, right? At least all the other Source links are.

@alexbiehl
Copy link
Member Author

alexbiehl commented Mar 21, 2018

Tests are failing still.

  • Mostly due to Ocean -> NewOcean churn
  • One test is more recent than the accepted tests in this PR

Currently I can't accept the new output as #767 blocks me here. Any volunteer who wants to accept the html-tests for this PR (@hvr or @gbaz maybe?)?

@gbaz
Copy link
Contributor

gbaz commented Mar 26, 2018

on this branch it looks like the datafiles need to be updated? in haddock-api.cabal?

@gbaz
Copy link
Contributor

gbaz commented Mar 26, 2018

Also nuno has some new assets for synopsis, plus, and minus that haven't been pulled in.

And I think there are a few more css changes he'd like to make, if people like them. (i.e. at least the font and footer stuff from haskell/hackage-server#731)

@alexbiehl
Copy link
Member Author

alexbiehl commented Mar 26, 2018 via email

@gbaz
Copy link
Contributor

gbaz commented Mar 26, 2018

Oh yes, I think I see the concern. @NunoAlexandre it looks like we've lost a lot of indentation in these cases, making things harder to visually distinguish. e.g.

before:
image

after:
image

The same occurs with methods and instances:

image

image

Also, in the above, the grey background of methods extends very far -- further even than the grey background of the instances bit itself. Weird...

@gbaz
Copy link
Contributor

gbaz commented Mar 26, 2018

Also, by the way, the new plus and minus signs look quite nice in the haddocks proper, but I'm not so sure how they look in the generated contents?

image

@gbaz
Copy link
Contributor

gbaz commented Mar 27, 2018

A few issues when swapping themes. First, if we use the new html with the old Ocean theme, the right-hand menu looks bad in the topbar (the text is pushed down, and since it is white, it is not visible at all:

image

A small tweak to the Ocean.css to fix this would be good.

Similarly for the Classic theme (not that anybody probably uses it):

image

(@NunoAlexandre what do you think about just reverting that small html change to the topbar and having the new css work with the existing one so we can leave prior stylesheets unchanged? I know the menu will not reflow quite as nicely, but it seems it might be simpler overall, and allow more free switching...)

Second, if we build docs with --built-in-themes to allow switching, there is no js on the html page anymore to allow switching, so we get "Uncaught ReferenceError: styleMenu is not defined" when we click on the style menu:

image

@alexbiehl
Copy link
Member Author

What the status here?

@NunoAlexandre
Copy link
Contributor

Hi @alexbiehl, I plan to do some work here this weekend 👍

@NunoAlexandre
Copy link
Contributor

@alexbiehl now this PR is on a branch on your own account. To edit this, I would need to fork your haddock repo and open a PR against it there. How do you suggest to do this instead?


@gbaz

A few issues when swapping themes. First, if we use the new html with the old Ocean theme, the right-hand menu looks bad in the topbar (the text is pushed down, and since it is white, it is not visible at all:

Why are we going to get our selves stuck because of such an old and unused theme? I don't quite like this theme toggle option idea either, to be honest. There's a reason we don't see it anywhere. One of the ideas behind this redesign is to have a consistent look, doing this is promoting exactly the opposite and making maintenance even harder.

In the worst case, we can update the old stylesheet to work well with the new html changes instead.

@alexbiehl
Copy link
Member Author

@NunoAlexandre I will give you access. Should be easiest, right?

@NunoAlexandre
Copy link
Contributor

Yes, thanks!

@NunoAlexandre
Copy link
Contributor

NunoAlexandre commented Apr 14, 2018

@alexbiehl @gbaz

I have updated this stylesheet with the latest state from hackage and have addresses other issues pointed out above, namely the indentation on code blocks.

@alexbiehl I could not reproduce you issue reported above (#782 (comment)). I tried pasting the latest CSS code into the live version of that page and the source link looks good.

You can have a look at some previews here.. Make sure to hard refresh to get the latest version of the stylesheet instead of a cached one.

About the backward compatibility point, I will wait for your response to my comment above.

Thanks!

@gbaz
Copy link
Contributor

gbaz commented Apr 14, 2018

In the worst case, we can update the old stylesheet to work well with the new html changes instead.

Nuno -- yes, this was my first proposal. I don't really care if we change the old css slightly or change the new html gen slightly, but it would be bad to break this feature for those who still want it. I'm happy with either solution you pick. It just seemed the case to me that the topbar html change was not that significant :-)

We really should fix the htmlgen to put back the theme-switcher js though. Again, it might not be the wisest feature, but it works, and its straightforward to keep working, and I guarantee there are people that will be frustrated if it stops being supported, so better to just cover their use case.

@alexbiehl
Copy link
Member Author

@NunoAlexandre great! One thing I noticed is that synopsis is still the old one.

bildschirmfoto 2018-04-15 um 20 00 31

@NunoAlexandre
Copy link
Contributor

@alexbiehl could you point me to a page where that happens? I can imagine an older version of the html causing that difference, but I have tried a few packages with the new stylesheet and it's always looking as expected.

@gbaz alright 👌

@NunoAlexandre
Copy link
Contributor

@alexbiehl could you also assign me to this PR? thanks!

@NunoAlexandre
Copy link
Contributor

I have adjusted the html changes so that it is backward-compatible.

I think now I just have to double check that different synopsis and I guess we are good to go after - at least for a first release.

@gbaz
Copy link
Contributor

gbaz commented Apr 21, 2018

@NunoAlexandre did you put back the js that lets the switcher work too? Also do you think the generated contents in the package index page look ok?

@NunoAlexandre
Copy link
Contributor

fd@gbaz I am not sure I understand the issue. I didn't remove any JS file or touched anything related to that switcher, neither did I ever see it live. Could you let me know where to look at and what's the background here?

Also do you think the generated contents in the package index page look ok?

I am not sure if by contents you mean the table of contents or just the whole #content part of the page?
Do you have a package you would like having me testing on? I am having issues with my environment to get this branch running and I am rather limited of time, just so you know.

Thanks!

@alexbiehl
Copy link
Member Author

@NunoAlexandre seems like your editor was a bit too eager with trailing whitespaces. These seem to be expected by haddock. (I will open an issue to strip whitespace from table cells)

@gbaz
Copy link
Contributor

gbaz commented Apr 25, 2018

@NunoAlexandre sorry it took me a while. re the switcher, the main point is described at #782 (comment). If you build haddocks with the --built-in-themes flag you get a "Style" menu in the top right, which lets you swap stylesheets on the fly. This requires some JS to be included in the page. Somehow that got lost. However, on investigation, it looks like this was already broken in the 2.19.0 release, so your changes are not related to the problem. I will file a separate issue for this.

Regarding "contents" I meant the module listing generated for each package, which I included a screenshot of at: #782 (comment)

@NunoAlexandre
Copy link
Contributor

@alexbiehl you are right, sorry :/ how can I get to build this branch? The easiest way to update the ref files is by running the tests and then move the ./html-test/ref/*.html files with html-test/out/*.html.dump (and drop the .dump extension). This is obviously not recommended by default, but in this case, it is fine as long as we make sure the only changes are the ones we expect to see.

Since I believe you are able to build this branch, and if you have the time, if you could do this yourself it would be great.

Nuno Alexandre added 4 commits May 19, 2018 13:02
- Fix and improve spacing
- Improve colors and borders
The current html generator of this branch wraps the package-header
caption as a div, which does not work (without style adjustments) with
the old themes. Changing it from div to span does the trick, without
needing to adjust the old stylesheets.
@NunoAlexandre
Copy link
Contributor

@alexbiehl since you can build this branch locally if you could update the ref tests yourself that'd be great (by moving the ./html-test/out/*.html.dump files to ./html-test/ref/).

I have tried following your suggestion but I have hiccups at every step. I tried installing a ghc-8.4-* but after that, the setup always failed due to parsing errors around build-depends. I thought that upgrading cabal would fix it but it didn't. Running cabal new-build exe:haddock results in:

Resolving dependencies...
cabal: Could not resolve dependencies:
[__0] trying: haddock-2.20.0 (user goal)
[__1] next goal: base (dependency of haddock)
[__1] rejecting: base-4.9.1.0/installed-4.9... (conflict: haddock =>
base^>=4.11.0)
[__1] rejecting: base-4.11.1.0, base-4.11.0.0, base-4.10.1.0, base-4.10.0.0,
base-4.9.1.0, base-4.9.0.0, base-4.8.2.0, base-4.8.1.0, base-4.8.0.0,
base-4.7.0.2, base-4.7.0.1, base-4.7.0.0, base-4.6.0.1, base-4.6.0.0,
base-4.5.1.0, base-4.5.0.0, base-4.4.1.0, base-4.4.0.0, base-4.3.1.0,
base-4.3.0.0, base-4.2.0.2, base-4.2.0.1, base-4.2.0.0, base-4.1.0.0,
base-4.0.0.0, base-3.0.3.2, base-3.0.3.1 (constraint from non-upgradeable
package requires installed instance)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: haddock, base

I've become a father recently thus my lack of time to work on this. Thanks already.

@alexbiehl
Copy link
Member Author

@NunoAlexandre no worries. We will take care of it.

(The error message indicates you are not building with ghc-8.4.* but with a ghc-8.0.*)

@sjakobi
Copy link
Member

sjakobi commented May 31, 2018

I have some time to spend on this, but I'm not sure what in particular I should look at. Any suggestions?

@sjakobi
Copy link
Member

sjakobi commented May 31, 2018

When I try to build the haddocks for these I get

≻ cabal new-haddock --with-haddock=/home/simon/src/haddock/dist-newstyle/build/x86_64-linux/ghc-8.4.2/haddock-2.20.0/x/haddock/noopt/build/haddock/haddock
...
Haddock's resource directory does not exist!

cabal: Failed to build documentation for these-0.7.4.

Not sure what to do about this.

@alexbiehl
Copy link
Member Author

alexbiehl commented May 31, 2018 via email

@sjakobi
Copy link
Member

sjakobi commented Jun 1, 2018

You need to pass haddock resource path to the invocation.
"-Lhaddock-api/resources.

Thanks, Alex! It's lowercase -l though.

@sjakobi
Copy link
Member

sjakobi commented Jun 1, 2018

I have accepted the changes in html-test and hopefully fixed the data-files section. The patch is at alexbiehl#1.

What I don't understand is why after changing the data-files section, html-test is again failing, but only when run with cabal new-test. See diff.txt.

When running

dist-newstyle/build/x86_64-linux/ghc-8.4.2/haddock-2.20.0/t/html-test/build/html-test/html-test --ghc-path=/opt/ghc/bin/ghc

I don't see these failures.

@sjakobi
Copy link
Member

sjakobi commented Jun 1, 2018

Regarding the styling I'm still seeing the following problems:

  • image

  • image

  • image


In this case I think the fonts in the Contents-box is relatively too small. But maybe scaling down the main font would be the preferable solution, as it seems quite large to me.

image


In this case the module info seems to take too much space at the top:

image


In this case a bit more indentation (not sure if that's the right word) for the docs would help each declaration stand out more.
image

@sjakobi
Copy link
Member

sjakobi commented Jun 1, 2018

@NunoAlexandre:

Regarding building and running the tests, try this or a variant

cabal new-test -w ghc-8.4.2 html-test

If ghc-8.4.2 is not on your PATH, you can pass in the full path to some ghc v8.4.*

@sjakobi
Copy link
Member

sjakobi commented Jun 10, 2018

What I don't understand is why after changing the data-files section, html-test is again failing, but only when run with cabal new-test. See diff.txt.

I think that was because I ran html-test without the --haddock-path option, so it ran with the wrong version of Haddock.

@gbaz
Copy link
Contributor

gbaz commented Jun 11, 2018

With regards to the problems, I think a number of assets still have not been properly moved into the correct location for this pr. cf: https://github.com/alexbiehl/haddock/blob/1289a62e9c15377ca591f176291c23d497b7e3d2/haddock-api/resources/html/NewOcean.std-theme/synopsis.png

In general, it should be double-checked that all the png assets in the NewOcean theme are actually the ones from Nuno and not just copied over from Ocean, as I think they all were in this PR.

@gbaz
Copy link
Contributor

gbaz commented Jun 11, 2018

@sjakobi outside of the asset issue, which really needs fixing, it seems your other comments are not obvious outright bugs, but rather design considerations with regards to spacing, size, etc? Is this correct? I'd like to triage to make sure we all understand the situation.

@sjakobi
Copy link
Member

sjakobi commented Jun 11, 2018

In general, it should be double-checked that all the png assets in the NewOcean theme are actually the ones from Nuno and not just copied over from Ocean, as I think they all were in this PR.

Right, but where are Nuno's png assets?

@sjakobi outside of the asset issue, which really needs fixing, it seems your other comments are not obvious outright bugs, but rather design considerations with regards to spacing, size, etc? Is this correct?

Well, I'd call the misplaced synopsis button a bug, but yes, what I pointed out are design issues.

@gbaz
Copy link
Contributor

gbaz commented Jun 11, 2018

Hm, it seems like its not an asset question per-se, but rather that the css just shouldn't be making use of that png at all. E.g. compare to the source at https://nunoalexandre.com/pets/haddock/Control.Lens.Operators.html where its clear that the two uses of synopsis.png in the css are vestigal and shouldn't actually be invoked from anywhere.

I guess something went a bit screwy in the merge?

With that synopsis style, I think the placement is not a bug, but correct? The problem is just that the css for it makes it look wrong at that location until we remove the weird png, etc.

@NunoAlexandre
Copy link
Contributor

I don't use assets, but just Unicode with content.

@NunoAlexandre
Copy link
Contributor

@gbaz @alexbiehl

This PR is dying. It was pretty ready to go until the decision of being backwards-compatible was made and delayed it all. Which, as I said above, seems pretty worthless to me.

Moving forward, how can we get this one merged? I didn't managet to get a build with the right ghc, and no proper documentation is to be found at all (or I didn't search well).

Could you document how to build and test this project using a specific ghc version?

I then can look into the comments raised about about the UI.

@harpocrates
Copy link
Collaborator

This PR is dying.

Let's revive it! The community obviously seemed to care about these changes, so it would be a shame to let them bitrot.

Could you document how to build and test this project using a specific ghc version?

With a recent version of cabal you should be able to just run the following to build and test. I just tried this right now and it worked (although there are tests failing due to whitespace differences).

$ cabal new-test -w ghc-8.4.2

If you don't have ghc-8.4.2 on PATH, you can specify the full path.

$ cabal new-test -w /usr/local/bin/ghc-8.4.2

If that doesn't work, could you paste in the error message you get?

@gbaz
Copy link
Contributor

gbaz commented Jul 30, 2018

Can someone summarize what still needs to be done?

Btw, if it makes things easier, mark suggests dropping support for style-switcher stuff in general... #810 (comment)

@gbaz
Copy link
Contributor

gbaz commented Aug 14, 2018

@alexbiehl I may have a little time to roll up my sleeves and help out here if you let me know what you think the situation is? (in terms of what remains?)

@NunoAlexandre
Copy link
Contributor

@gbaz if the style-switcher is removed, we can just drop the last two commits of this branch, meaning the head becomes 4aa56dd.

Tweaking wise there are some points raised but nothing major stands in the way.

@luc-tielen
Copy link

Any update on this? Seems like it's been some time since last post on this PR.
Really appreciate the new look of hackage, would be cool if the haddocks would also get these style updates! 😄

@harpocrates
Copy link
Collaborator

Superseded by #949.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants