Skip to content

Refresh HTML manual/API docs style#12868

Merged
gasche merged 3 commits intoocaml:trunkfrom
yawaramin:simpler-doc-style
Jan 6, 2024
Merged

Refresh HTML manual/API docs style#12868
gasche merged 3 commits intoocaml:trunkfrom
yawaramin:simpler-doc-style

Conversation

@yawaramin
Copy link
Contributor

@yawaramin yawaramin commented Dec 29, 2023

Simplify colours and get rid of gradients. Also fix search button icon.

Before:

Screenshot 2023-12-29 at 11 00 37

After:

Screenshot 2023-12-29 at 01 01 14

@sidkshatriya
Copy link
Contributor

sidkshatriya commented Dec 29, 2023

Suggestion: please attach a before screenshot for easy comparison. So it can be compared visually with the after screenshot you posted. This way we don't need to actually go to the site to compare with this.

Also screenshot to screenshot comparison is easier since the size, dpi, color temperature etc. would be the same in both.

@yawaramin
Copy link
Contributor Author

After further reducing the top border of each item:

Screenshot 2023-12-29 at 11 02 18

@wikku
Copy link
Contributor

wikku commented Dec 29, 2023

That's a big improvement IMO.

The top border clearly delineates items and communicates that I should be reading the description below and not above, currently only indicated by slightly more whitespace below descriptions. Though I'd use border-top: 2px.

The screenshot still shows odd alignment of the description and "Raises" warnings, I think aligning them both with margin-left: 1.5 rem would look good.

image
(This was just playing with Firefox dev tools)

@yawaramin
Copy link
Contributor Author

Thanks! Adjusted description items left margin:

Screenshot 2023-12-29 at 12 22 22

Copy link

@SGrondin SGrondin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this change there's no need to download the search_icon.svg anymore, so it should probably be removed from the Makefile: here and here

Note: I agree, I think it's a huge improvement

@yawaramin
Copy link
Contributor Author

Awesome, thanks! Removed from Makefile, and no wonder I couldn't find that file in the repo.

Copy link

@SGrondin SGrondin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the Makefile change it looks good to me

@yawaramin yawaramin changed the title Refresh HTML manual/API docs style [WIP - do not merge] Refresh HTML manual/API docs style Dec 30, 2023
@yawaramin yawaramin marked this pull request as draft December 30, 2023 18:05
@yawaramin
Copy link
Contributor Author

Looks like the manual and API docs styles are entangled. Need to fix this first.

Screenshot 2023-12-30 at 13 05 40

@yawaramin yawaramin changed the title [WIP - do not merge] Refresh HTML manual/API docs style Refresh HTML manual/API docs style Dec 30, 2023
@yawaramin yawaramin marked this pull request as ready for review December 30, 2023 20:21
@yawaramin
Copy link
Contributor Author

Manual:

Screenshot 2023-12-30 at 15 22 26

API docs:

Screenshot 2023-12-30 at 15 22 51

@gasche
Copy link
Member

gasche commented Jan 2, 2024

For code blocks in the manual I don't understand why you are adding a top border. In the doc it makes sense because the item is always related to the doc that follows, not to the content before, so you add a visual separator above. But this is not true for arbitrary code blocks in the manual, including in the examples in your screenshot -- fib is related to the text right above it.

@yawaramin
Copy link
Contributor Author

I guess for consistency with the API docs, but you have a point that the delineation separates the prose from its related code. How about a bottom border instead? I feel like some structure in the code formatting is helpful.

Screenshot 2024-01-02 at 16 44 45

@Octachron
Copy link
Member

Octachron commented Jan 3, 2024

Thanks for the changes. The changes look good to me for my non-expert eyes.

I think we want to avoid visible horizontal lines as delimiters on the manual side, both top and bottom since the code
is often part of the text there. Maybe adding a vertical border on the left or right side could work better?

Looking at ocaml.org for another inspiration, the tutorial are using round border (but with syntactic color highlighting).

@yawaramin
Copy link
Contributor Author

Thanks @Octachron for the review. I'm not terribly attached to the borders so just removed them for the manual. Now it looks like this:

Screenshot 2024-01-03 at 19 07 26

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result looks good to me now. Approved.

There are a few places in the code where the PR appears to break indentation, could you fix them?

Can you clean the history to avoid the back-and-forth?

background-color: #edbf84;}
background-color: white;
text-decoration: underline;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the identation looks off here


pre{
background:linear-gradient(to left,#fff 0,#ede8e5 100%)
background-color: #ede8e5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation

border-radius: 1px;
/*border-bottom: 4px solid rgb(255, 215, 181);*/
box-shadow: 0 4px 0 0px rgb(255, 215, 181);
background-color:#ffd6b5;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation

Simplify colours and get rid of gradients. Also fix search button icon.
@yawaramin
Copy link
Contributor Author

@gasche sure, did the requested clean-ups.

@gasche
Copy link
Member

gasche commented Jan 4, 2024

@Octachron do you want to have another round of feedback? Otherwise please feel free to merge.

@gasche
Copy link
Member

gasche commented Jan 4, 2024

(It would be nice to have the changes in the 5.2 version of the manual)

onclick = "this.oninput();"
onpaste = "this.oninput();">
<img src="search_icon.svg" alt="Search" class="api_search svg" onclick="mySearch(%b)">%s</div>
<button title="Search" onclick="mySearch(%b)" style="cursor:pointer">🔎</button> %s</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises a small concern in term of accessibility. I wonder if replacing 🔎 with

<span aria-hidden="true">🔎</span>Search

would not be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, I've just done this. Looks like:

Screenshot 2024-01-04 at 13 37 14

@gasche gasche merged commit 784db4e into ocaml:trunk Jan 6, 2024
@gasche
Copy link
Member

gasche commented Jan 6, 2024

Merged, thanks!

@yawaramin yawaramin deleted the simpler-doc-style branch January 6, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants