-
Notifications
You must be signed in to change notification settings - Fork 96
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
[DOC] Freshen up the homepage #1062
[DOC] Freshen up the homepage #1062
Conversation
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot @Vincent-Maladiere this is super cool! 🎉
you may want to check how it looks on narrow screens or mobile phones:
- there is no padding in the top banner so the last of the 3 bullet points touches the bottom of the banner
- there is too much white space on each side of the screen due to the 80% max-width
- the gaps between the sections need to be tightened IMO
I'm not sure why there is no line between "effortless pipelines" and "powerful feature engineering" but there is one between the other sections?
I think we need to create the tablereport as part of the sphinx build process, rather than adding it to the repo. it will avoid having huge meaningless diffs in the repo and also this way when we improve the report the homepage will get updated without needing to regenerate it and add it manually. (we may also be able to do the highlighting of code snippets with pygments during the sphinx build to be able to write them in python and update them more easily). I'm ok with addressing this last point in another PR though
I don't see it, can you provide a screenshot? The horizontal lines split all feature sections.
I'm afraid I have to disagree with both points. Having a small margin on a desktop is awkward. Spacing between sections also prevents it from being too crowded with code and demos. I agree with you on mobile, however. I reduced them for this format. I also changed the orange color of the button and headers to bump the contrast ratio to 3.33, which should be enough for large texts. I can't do more than that, because it wouldn't be orange any more. TODO:
|
I think it is due to the 0.5px line width, the lines appear and disappear depending on the zoom level. on my laptop and external screen at the default zoom 100% on firefox it doesn't show up between those 2 sections. that's not a big deal either though |
sorry i wasn't clear, for the spacing between sections I meant on a phone screen only, it looks fine to me on a big screen. (I saw you changed it already for small screens 🚀 ) |
thanks! If needed we could always make it not orange but I think it looks ok now. the hovered state still has a very low contrast but that's probably less important |
One "graphical" detail: the new homepage is made of horizontal blocks. It would be very nice to have one out of two with a slightly greyed background. I do realize that this implies removing the "hr" elements and using the background color of the divs instead. |
Feedback applied so far:
|
doc/_static/gap.png
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use an SVG here? Matplotlib saves very nice SVGs (I think that you need to use ", transparent=True" to savefig to have a transparent background, which comes in handy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Gael Varoquaux <[email protected]>
Can we easily add a vignette saying "And many more" at the end of the contributor list? When clicked it would go to the github contributor list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot @Vincent-Maladiere !! I think it highlights the selling points of skrub really well.
I think the blue/orange banner at the top is too tall on narrow displays and ends up pushing the "getting started" button out of the first screen, but that is somewhat orthogonal to the changes done here so it could be addressed in another pr
doc/_templates/index.html
Outdated
{% include "demo_table_report_code.html" %} | ||
</div> | ||
<div class="col-md-7"> | ||
<p class="click-table-report-hint">Click anywhere on the table!</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a better suggestion but with "click on the table" people might look only at the table and miss the tabs and the column filter... maybe something like "the displayed report is interactive"?... I can't come up with a better wording and it's not a very big deal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about moving the text back to the left panel (for better usage of the space), and writing "Try it out: click anywhere ➔"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for me, except for the arrow because the report will be below the text on a small screen and not at its right.
The issue with adding a grey background to some sections is that their margins would remain white (notice how the line breaks don't fill the entire width on big screens). That is because I can't get rid of the main container {% block docs_main %} defined somewhere in Jinja & Sphinx. This container has set some margins that I can't style. I suggest adding an issue to try that after this PR because this is quite challenging. WDYT @GaelVaroquaux ? |
@jeromedockes writes:
Indeed, that's a good illustration of why we're always fighting for screen real estate :) I think that the third bullet point can be made shorter: "Work on heterogenous types (numeric, categorical, dates, text, missing values...)" -> "heterogenous data (numbers, categories, dates, text, missing values...)" |
Or this one? https://fontawesome.com/icons/arrow-right?s=solid |
I think Gael's trick was to have an arrow that points towards the south-east so that it's in the right direction whether it is above or on the left of the report. if there isn't one in fontawesome we can just add a small svg file (and it will probably look better, too) |
Haha, hacky indeed! Let me try that |
in the custom.css there is a |
Yes, but more specifically the issue I have is maintaining the gradient band as it is. I can vertically align the h1 and h4 with the text below, but it creates a white margin on the left and right of the band. Edit: removing sk-landing-container entirely solves the problem Also, I think putting the button on the right cuts the natural flow of a user landing on the page. It doesn't feel natural to read text on the left and then cross the spread horizontally to see/click the button. I'm -1 for this tradeoff with the vertical scrolling. |
Side comment: if you want inspiration on a good page (done by professionals with a lot of money) https://spark.apache.org/ It's an awesome tradeoff in terms of information density and vertical scrolling |
They did an amazing job indeed |
I would move the "Our community" text on the side |
Also: the first line is done with a pair of "md-4" / "md-8" divs. As a consequence, the div where there is the text is much smaller than that where there is the button. I would use "md-6" / "md-6" to have the same size, or "md-8", "md-4" to have a bigger div for the text: it makes reading the text easier |
How about moving the "Click anywhere on the table ↓" to the side? |
What do you mean? Isn't it already on the side? |
Do you mean on the left side? Below the code? |
> I would move the "Our community" text on the side
What do you mean? Isn't it already on the side?
The whole text in the right panel
> How about moving the "Click anywhere on the table ↓" to the side?
Do you mean on the left side? Below the code?
Yes indeed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Thanks a lot for all these changes.
Let's merge
I like the sponge logo a lot btw :) |
If we can simplify the sponge enough to have it as a favicon, it could be a good replacement for our current logo. |
I guess it could be a uniform yellow S without any of the decorations? |
* first version of the new homepage * enhance feature engineering wording * fix footer * split the index.html file into demo html files * remove unused style class * apply some feedbacks * apply feedback * apply jerome feedback on max-width * apply ricardo feedback on hero wording * Update doc/_templates/index.html Co-authored-by: Gael Varoquaux <[email protected]> * add 'and many more' link * generate table report only once * Geal feedback reduce scrolling * fix the vertical alignment issue * justify left community section * use col-md-6 / col-md-6 for the hero * add diag arrow * put community title and text on the right side --------- Co-authored-by: Gael Varoquaux <[email protected]>
This reverts commit 3f06b90.
This PR revamps the homepage to articulate what skrub does more clearly.
From top to bottom: