Skip to content
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

Fix/themeimprovements #17

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

drc0
Copy link

@drc0 drc0 commented Apr 10, 2023

addresses issue #16

@zzzeek
Copy link
Contributor

zzzeek commented Apr 13, 2023

can you confirm this supercedes #15 which we can close?

Copy link
Contributor

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

OK this is very good and I definitely want to go in this direction. Looking through here are a bunch of things that I noted, I'm not sure to what degree each are intentional or not but these are all the things I'd want to adjust further. These are mostly all the same thing which is that the left border between the sidebar and the start of the content is too wide, and has a slight two-tone white change in color that should happen at the border and not to the right of the border

slight change from white to slightly less white - I'm not sure that you can see this but there overall seems to be a very slight change in white hue from the sidebar over to the main content that looks awkward, the "white" color should likely all be the same to the right of the vertical sidebar border (This is also visible in the mobile screenshot in the next item):

Screenshot from 2023-04-13 17-20-33

spacing on sides of mobile view appear to be uneven - the old design was like this also in a different way, but because it had lots of boxes, things still looked "even" relative to the different borders. now that we have less boxes, it would be better if the text and codebox looked centered, for a top level section (also the white/slightly less white thing gets in the way here) - note especially the codebox has a lot more space on the left than on the right, this should be straight for top level. it looks like the right margin moved out:

Screenshot from 2023-04-13 17-20-10

more white hue changes

this is all the same white hue change thing happening as above, same fix

the change from slightly gray on the left to white on the right should happen at the border line, the gray shouldn't slide throguh the border because it creates a second "border" of the white hue changing

Screenshot from 2023-04-13 17-20-52

another image of a box that looks shifted over to the right w/ unusually large two-tone white hue change on left side

Screenshot from 2023-04-13 17-22-40

pipe after release badge seems to have lost spacing

Screenshot from 2023-04-13 17-23-48

@zzzeek
Copy link
Contributor

zzzeek commented Apr 13, 2023

i think this is much nicer to read so i'd like to get this up ASAP!

Copy link
Contributor

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

index page - right side of the top box does not go far enough, has a light gray "tag" hanging off of it

Screenshot from 2023-04-13 17-39-41

errant fragment of a yellow box on the bottom of index page

Screenshot from 2023-04-13 17-41-02

vertical position of left text vs. right text in top toolbar is not even

image

Copy link
Contributor

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

inconsistent style between sidebar version of "topnav / search" and index page "topnav / search" - probably should do the white borderless style on the index page also

image

image

@drc0
Copy link
Author

drc0 commented Apr 14, 2023

Wow thanks for the feedback!
regarding #17 (review) yes that is intentional, when you see the color changes from

gray , darker gray border, white

that was choosen to accentuate the separation between the background and the white content, usually giving a border on that box makes the box more visibile and less 'floating', you can try from the browser inspect tool to remove the border rule and see if you like it more or not, this is more evident on code (pre) boxes, but I'm willing to make the change if you like it more without the border!

In the meantime I'm going to address the bugs you listed and come back with a fix (ideally after the weekend)

  • mobile view uneven spacing
  • fix padding from sidebar to content, make that even so that the content does not seems shiftet to the right
  • release badge theme and spacing with the pipe
  • search box in index page has an hanging border and does not stretch fully 100%
  • yellow bg on the bottom of index page
  • vertical positioning in header section, title from release info
  • improve consistency between sidebar nav and index page nav

thanks!

@zzzeek
Copy link
Contributor

zzzeek commented Apr 14, 2023

re: the border transition, the main content is #ffffff, pure white, and the overall background of the rest of the page is #fdfbfc, slightly off white. From what I can see there's no "gray" box in the layout, the sidebar seems to be inheriting the #fbfbfc from the overall page. I've scoured the web for some quantitative advice on background colors changing in this way to suggest different areas of content but have not found anything, however, the contrast between these two colors for me personally is way too low to not be disturbing without a clear box directly where the color changes. That is, the color change is too subtle to look intentional and it looks like a mistake, and it also makes it seem cluttered like there are two vertical partitions to jump through between the two sections, making the content look ambiguously uncentered especially in mobile (but you indicated you can fix the centering without changing the color transition scheme...OK).

if we look right here at Github, the right sidebar is not any different color at all, it's the same white as the rest of the page, and the entire page is #ffffff pure white. I think if we had just a "suggestive border line" like you have then there would be no change in color, or a much higher contrast change in color. MySQL's docs also, they have the kind of border line you are doing here, kind of a suggestive line that's not quite a box: https://dev.mysql.com/doc/refman/8.0/en/language-structure.html the two background colors between sidebar and content are the same.

so here's all the possibilities I can think of:

sidebar / content / border

white = #ffffff
off white = #fdfbfc
gray = #efefef

box = a box that edges exactly where the color changes
line = a line that flows down inside the box a bit

bold = seems better to me / can find examples

  • white / white / box
  • white / white / line
  • off white / white / box
  • off white / white / line
  • off white / white / none
  • gray / white / box
  • gray / white / line
  • gray / white / none

If we can fix the uneven centering issues while still having the two-tone + offset line pattern, I'm not sure how you can do that but I would be interested in seeing that too.

@zzzeek
Copy link
Contributor

zzzeek commented Apr 14, 2023

that said if you can fix the "uneven centering" issues without changing the color scheme you have, maybe we try that first so I can see at least that part of it.

@zzzeek
Copy link
Contributor

zzzeek commented Apr 18, 2023

I really love this look! hoping to see the next iteration :)

zzzeek added a commit to sqlalchemyorg/sqlalchemyorg that referenced this pull request Apr 19, 2023
For link colors to change in the docs we need to change them
here, where the same rules apply based on the contrast ratio
we looked up.

References: sqlalchemyorg/zzzeeksphinx#17
zzzeek added a commit that referenced this pull request Apr 19, 2023
our hyperlink color fails color contrast guidelines,
so fix that in conjunction with the main site so that
link color is brightened.

Additionally anticipate a few changes that were set up
for PR at [1], where we lose most outer borders and push
boxes outwards to touch each other.  We mostly want to get
immediate readability changes out, which we can continue to refine.

At the moment we are drawing some ideas from
https://docs.djangoproject.com/en/4.2/intro/tutorial03/ where
the various areas still have different background colors, but borders
are mostly non present and there is no white margin between
different sections.

[1] #17
@zzzeek
Copy link
Contributor

zzzeek commented Apr 19, 2023

So I was anxious to get the brighter link color up and also to cut down on the "lines" everywhere - i was not aware we had design elements that were impacting accessibility and i wanted to get a couple of the basic things up. The brighter link color on the site ultimately comes from the site's main css so i had to update it over there.

so I made about 10% of the changes which you can see at https://docs.sqlalchemy.org/en/20/orm/mapping_styles.html which include:

  1. brighter link color (though not as bright as it is here, I made the main font color darker to keep the ratios)
  2. remove drop shadows entirely
  3. remove 1px borders from outer headers / sidebars / main content
  4. grow outer headers /sidebars to touch the edges of each other as well as the page

So the idea to grow out the headers/sidebars to touch without borders, but still use the colors, I got from Django's docs which we can see here: https://docs.djangoproject.com/en/4.2/intro/tutorial03/ . django's site is overall a "full width" kind of site which ours currently isn't, the overall site still has borders on the sides unless you shrink the page down.

I didnt make any changes to how fonts / header text is laid out or any of that and I think the subsequent changes in the PR here are good, if you have the resources to keep working on them. thanks for helping with the site!

@zzzeek
Copy link
Contributor

zzzeek commented Apr 19, 2023

still playing with widths, I can see the width between left sidebar and content is too small

@drc0 drc0 force-pushed the fix/themeimprovements branch 2 times, most recently from 859bf80 to 8cf7e27 Compare April 22, 2023 13:43
@drc0
Copy link
Author

drc0 commented Apr 22, 2023

Hello @zzzeek! I'm happy on the direction you choosed for the docs, that is already a lot better in my opinion.
I agree that the django documentation is clear and well organized, I always found that doc a pleasure to read, I like a lot their choice of dotted link underlines, and their color choice that is not too distracting even if the important elements are distinguishable.

I have refined this PR addressing all the issues and made spacing changes on bigger screens, overall the new design is like the screenshot below.

I hope that even if you will not merge as is it could provide a good direction.

(also consider that this patch must be viewed with also this pr because of various styles collisions sqlalchemyorg/sqlalchemyorg#15)

thanks!

immagine

@CaselIT
Copy link
Contributor

CaselIT commented Apr 22, 2023

Thanks for the updates. A few comments on my part:

  • can you rebase / merge master since there seem to be a few conflicts?
  • I think the side nav could avoid being vertically cut, unless it always stays in the view when scrolling
  • not sure if I like the dotted underline in the links, but I can live with them
  • the "Note to ORM readers" could probably use a border
  • I think there is too much space between notes and code. I think about half that would look nicer.
  • I think there is too much space around the titles. I think at the top we could remove a 30%, while at the bottom we could make it match the space between text and code

Overall though it looks great, thank you for the effort you have and are putting into it!

@drc0
Copy link
Author

drc0 commented Apr 23, 2023

Hello @CaselIT !

  • I was considering the rebase but probably that would be equivalent to "push -f" given the different layout directions on some points
  • Yes in reality the sidebar is fixed and the screenshot can't give the correct feeling
  • ok!
  • Regarding the last three points, I would suggest (even if somewhat difficult but I'm following the instructions given here improve documentation legibility #16 (comment)) to try out the new layout navigating it, the screenshot is a bit zoomed in (probably you must set it at 80% size) and overall the feeling after the first impact is a bit different from the current one online. The spacing choices give more room to the eye while scanning the doc, and the absence of borders helps after we spend some time on the docs. If even after this consideration you feel there is a little too room room for the eye I would space it down on smaller screens (laptop down maybe).

thanks!

@CaselIT
Copy link
Contributor

CaselIT commented Apr 23, 2023

Regarding the first point, you could also just merge with main, so no force push is needed

Ok I'll try the site, and report back

@CaselIT
Copy link
Contributor

CaselIT commented Apr 24, 2023

Ok I'll try the site, and report back

can't get the site to build correctly, I've followed instruction here #16 (comment) but does not work.

Going from the sqlalchemyorg website on 8000 to the docs returns a 404 on http://localhost:8080/docs, while loading directly http://localhost:8080 does not load the main side style.

@zzzeek there is a script that runs both correctly?

@zzzeek
Copy link
Contributor

zzzeek commented Apr 24, 2023

you have to visit the two hosts individually, run sphinx with "make autobuild" and view the docs under localhost:8080. the cross-linking between the two sites in practice is partially dependent on apache .htaccess and mod rewrite rules

@CaselIT
Copy link
Contributor

CaselIT commented Apr 24, 2023

Ok, but if I go to the sphinx output site I don't have the main site header and the main site css

@zzzeek
Copy link
Contributor

zzzeek commented Apr 24, 2023

you have to build sphinx like this:

READTHEDOCS=True RTD_SITE_BASE=http://localhost:8000/ make clean autobuild

then the static pages will be built using templates from localhost:8000, with live link to css from localhost:8000

@CaselIT
Copy link
Contributor

CaselIT commented Apr 24, 2023

I'll give it another try. I thought I did that.

@zzzeek
Copy link
Contributor

zzzeek commented May 5, 2023

sorry we havent had time to look at this. Since I put up the changes that were most urgent, like the link color, I have been working on other things.

Overall with the docs, I've been working on them for like 16 years, and still I agree with everyone that something "feels" off with how they are done. I have been trying over and over again to fix that "feel" but I feel like I keep missing. This week I spent a lot with https://textual.textualize.io/ which has very nice, modern docs, but also more barebones (more than I'd prefer for us). It's not the same kind of library as ours but their search seems to be a lot better. Doesn't seem to be using sphinx. Not like I want to copy them, we aren't the same kind of library, but just interesting to see how it felt to learn a new library from their docs where I had to learn new concepts and things.

@CaselIT
Copy link
Contributor

CaselIT commented May 5, 2023

Textual seems to be using https://squidfunk.github.io/mkdocs-material/ for the docs.

Also sorry but I've still to try the docs. I'll try doing that once I've a bit of time

@CaselIT
Copy link
Contributor

CaselIT commented May 12, 2023

ok I've finally managed to try and make the site build correctly.

Hello @CaselIT !

* I was considering the rebase but probably that would be equivalent to "push -f" given the different layout directions on some points

* Yes in reality the sidebar is fixed and the screenshot can't give the correct feeling

* ok!

* Regarding the last three points, I would suggest (even if somewhat difficult but I'm following the instructions given here [improve documentation legibility #16 (comment)](https://github.com/sqlalchemyorg/zzzeeksphinx/issues/16#issuecomment-1500998882)) to try out the new layout navigating it, the screenshot is a bit zoomed in (probably you must set it at 80% size) and overall the feeling after the first impact is a bit different from the current one online. The spacing choices give more room to the eye while scanning the doc, and the absence of borders helps after we spend some time on the docs. If even after this consideration you feel there is a little too room room for the eye I would space it down on smaller screens (laptop down maybe).

thanks!

I kinda still think it would be better with my point above.
In particular:

the "Note to ORM readers" could probably use a border
I think there is too much space between notes and code. I think about half that would look nicer.

At the moment there are 50px top and bottom. Let's cut that to 25 (or 1.5 em) Same for the admonitions (the tip)
The code box should restore the internal padding, like in the site now of something like 10px.
I also think that the code background is too light, something closer to the current version would be better.
For example in the image below there is too much white space (from changelog/migration_20.html)
image

I think there is too much space around the titles. I think at the top we could remove a 30%, while at the bottom we could make it match the space between text and code

there is way too much space. The top is almost 100px (92.4). Let's cut that to max 2em or 32px. We could to: titles 1em top and bottom. End of section 1em bottom.

Some additional things:

  • is seems that there is a line that's at the top of the titles in pages like orm/quickstart.html. It's there also in the old site, but it's now closer
    image
  • personally I think 1rem at the bottom of each p is a bit too much, but I see that the live site is already like this. .75 looks nicer imho

Overall the update is nice. Thanks @drc0. Let us know if you have time for updating the site or if we should take over.

Thanks for the work done so far!

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.

3 participants