-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
WEB: Adding new pandas website #28014
Conversation
Looks good. Very clean layout. A few comments: 2: Is TideShift a Sponsor of Pandas now? Then they should be on the "with the support of..." list? 3: The link to Wes McKinney's book is http://www.kqzyfj.com/click-7040302-11260198?url=http%3A%2F%2Fshop.oreilly.com%2Fproduct%2F0636920050896.do&cjsku=0636920023784. Is there a reason for going through that |
Thanks for the review @topper-123, good feedback. I added Tidelift to the list of partners, and the docs links not working is intentional. I'm planning to add them after this is merged, and I make the docs use this same layout. Now they'd simply link to the current docs, and you'd leave the website. Regarding Wes book, I guess that redirect was to keep track on the number of clicks. But besides looking fishy, clicking on it asks me to confirm that I accept some weird conditions, and after that it takes me to the home page of O'Reilly, not the book page (I guess that's because the 2nd edition has been released)_. I updated the link to the direct link to the second edition, and also change the image to be the second edition front page. @wesm if you want to track clicks to the book, I think the best is to create a page in our own website that performs an automatic redirect, and track the visits with Google Analytics. Let me know if you want me to implement that. |
web/pandas/index.html
Outdated
</div> | ||
<h4>Get the book</h4> | ||
<p> | ||
<a href="http://shop.oreilly.com/product/0636920050896.do"> |
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.
If you want to change this to an affiliate link (so we receive ad fees), here is the one: https://amzn.to/2KI5JJw. I can donate annual proceeds from this link (probably not a lot in aggregate) to NumFOCUS
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.
That makes sense, thank you. Updated.
Thanks for the feedback @mroeschke. I'll let the designer from Indeed take care of that. The website uses bootstrap, and should be very easy to make it responsive and look perfect in mobile. But I don't know exactly how it works, so I'll let her do it. |
Removed the carousel, and made couple of changes to reduce the size of the website files. There are couple of things I want to change to reduce the size further. After that, if there is no particular change requested, I think we should merge and continue with follow ups. This way we can work in parallel with the integration with the docs and the CI to display the dev version of the web. The pending changes are:
|
…r project is interested in contributing it upstream
I made the web responsive (couple of things still need to be changed to look perfect in mobile, but almost there). Also removed the "try pandas" page for now and the blog (will focus on the core here, and add those as a follow up). The version in https://datapythonista.github.io/pandas-web/ is almost the one I'd like to get merged here, and then work from there in other PRs. Any feedback welcome. |
Glad to hear it. Roughly speaking, what's the total size of the new files / assets here? |
Just glanced briefly from phone where it looks really good. Just as a heads up though I think all of the doc pages are broken - at least navigating through gives a 404
https://datapythonista.github.io/pandas-web/docs/user_guide/
Sorry if I missed this on the call yesterday but how does this fit into what Stijn is doing? Would you want this merged as a precursor or does it replace some aspects?
…Sent from my iPhone
On Sep 12, 2019, at 9:43 AM, Tom Augspurger ***@***.***> wrote:
Glad to hear it.
Roughly speaking, what's the total size of the new files / assets here?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
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.
Is the new web/pandas/static/img/pandas.svg
the same as doc/logo/pandas_logo.svg
?
- [Setting up a development environment](setup.html) | ||
- [Working with git](git.md) | ||
- [Contributing to the documentation](docs.html) | ||
- [Docstring guidelines](docstring_guidelines.html) |
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.
Looks like nested lists aren't handled properly? https://datapythonista.github.io/pandas-web/community/contributing/
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.
Looks like markdown requires 4 spaces not 2, fixed
to use 4 spaces for tabs. | ||
|
||
--- | ||
The steps below will download around 900Mb for the pandas |
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.
Is 900Mb accurate?
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.
Looks like it's 150 Mb for the repo, and 74 for conda dependencies. Not sure why those were wrong (that part is from the worldwide sprint). Fixed
web/pandas/community/ecosystem.md
Outdated
- ``vaex.from_pandas`` | ||
- ``vaex.to_pandas_df`` | ||
|
||
## Data validation |
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 think pyjanitor was recently added. May want to check for recent commits to these pages.
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.
Updated, just couple of changes in the copied files.
I think these two can proceed mostly independently.
Marc's is updating the website. Stijn's is updating the docs.
We'll need to reconcile the styles by using a single CSS file for both, but
that can be done afterwards I think.
On Thu, Sep 12, 2019 at 11:46 AM William Ayd <notifications@github.com>
wrote:
… Just glanced briefly from phone where it looks really good. Just as a
heads up though I think all of the doc pages are broken - at least
navigating through gives a 404
https://datapythonista.github.io/pandas-web/docs/user_guide/
Sorry if I missed this on the call yesterday but how does this fit into
what Stijn is doing? Would you want this merged as a precursor or does it
replace some aspects?
Sent from my iPhone
> On Sep 12, 2019, at 9:43 AM, Tom Augspurger ***@***.***>
wrote:
>
> Glad to hear it.
>
> Roughly speaking, what's the total size of the new files / assets here?
>
> —
> You are receiving this because you are on a team that was mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#28014?email_source=notifications&email_token=AAKAOIU44MQQLID6UEJVLHLQJJW7XA5CNFSM4INCLXP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6SQXEQ#issuecomment-530910098>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIX6D3HK3FGLZKRM4Z3QJJW7XANCNFSM4INCLXPQ>
.
|
Regarding size, I think it's around 500kb, but I still want to optimize the images in the Install page (the rest is almost nothing). The 404 for the docs are expected. The plan is to merge this and then start integrating both, so there is no perception from the user that they are built separately. There are some challenges there, but spoke with Joris at EuroSciPy, and seems doable. |
There is also an issue with sizing of content on the Contributors page due to long user names. Maybe OK to just have names and link to GH site if people wanted
Sounds good, though I'm wondering if we should commit to one style as part of this change then. Seems like it will be more hassle in the long run if the docs and rest of the site are built with two different style templates in mind and then we try merging later |
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.
In addition to the inline comments, the pandas_fibonnaci image can be removed I think?
web/pandas/_templates/layout.html
Outdated
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<title>pandas</title> |
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.
<title>pandas</title> | |
<title>pandas - Python Data Analysis Library</title> |
web/pandas/_templates/layout.html
Outdated
$('[data-toggle="popover"]').popover() | ||
}) | ||
$(function () { | ||
// temporary functionality to test different logos |
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.
Remove this before merging?
web/pandas/_templates/layout.html
Outdated
crossorigin="anonymous"></script> | ||
<script> | ||
$(function () { | ||
$('[data-toggle="tooltip"]').tooltip() |
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.
Can you add a comment for what those functions are needed?
web/pandas/_templates/layout.html
Outdated
$(".navbar-brand img").attr("src", img); | ||
$(".navbar-brand img").height(30); | ||
}); | ||
</script> |
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.
To add to a todo list: add google analytics
year={2011} | ||
} | ||
|
||
## Brand and logo |
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.
Once we have a new logo, this probably deserves it's own apge
web/pandas/community/license.md
Outdated
@@ -0,0 +1,31 @@ | |||
# License | |||
|
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.
For me, this doesn't add much value as separate page on the website, I would say
I am trying it out locally, but the images are not working for me. Changing Also the links in the navbar don't work locally. -- Can you add a README in the |
Addressed comments. You need to run |
Thanks! Can you add that note to the readme / command help? |
web/pandas/_templates/layout.html
Outdated
<span class="navbar-toggler-icon"></span> | ||
</button> | ||
|
||
<a class="navbar-brand" href="{{ base_url }}/"><img alt="" src="{{ base_url }}{{ static.logo }}"/></a> |
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 not working at the moment (I think static.logo is not defined right now?)
Another thing: the salsalabs donate container is not working (not locally, but also not on your hosted version) |
…allation instructions
The iframe works for me, not sure if it was a temporary problem in their side, or you've got an adblocker or something preventing it to render. |
I didn't want to include the logo, to avoid increasing the size of the repo with something will be replaced soon. I'm removing the logo from the html for now, so it doesn't show broken. |
Linking to the logo file in the docs does not work? (and I suppose on build copying that to the build directory) |
That logo has the font in black, I could link it, but I'd have to change the header to white to look ok. I think we still need several iterations with the website until we want to publish it to production, so I would focus on having a starting point so we can open PRs to it, and not in trying to have something final (it's far from it anyway, starting by the home page). |
(function() { | ||
var ga = document.createElement('script'); ga.type = 'text/javascript'; ga.async = true; | ||
ga.src = ('https:' == document.location.protocol ? 'https://ssl' : 'http://www') + '.google-analytics.com/ga.js'; | ||
var s = document.getElementsByTagName('script')[0]; s.parentNode.insertBefore(ga, s); |
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.
Can you use the same one as for the docs?
pandas/doc/source/themes/nature_with_gtoc/layout.html
Lines 98 to 106 in 372a9a0
<!-- Google Analytics --> | |
<script> | |
window.ga=window.ga||function(){(ga.q=ga.q||[]).push(arguments)};ga.l=+new Date; | |
ga('create', 'UA-27880019-2', 'auto'); | |
ga('set', 'anonymizeIp', true); | |
ga('send', 'pageview'); | |
</script> | |
<script async src='https://www.google-analytics.com/analytics.js'></script> | |
<!-- End Google Analytics --> |
(or is there a reason the above is better? then we can update the docs ones)
Also, can this go at the bottom scripts? Or is there a good reason to load this first?
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 copied the one from the website, didn't know the docs had a different one.
I followed Google instructions on where to place it, they say immediately after the <head>
tag.
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 recently update the docs one, it was on my to do list to also update the website but didn't get to that yet ... (kind of proving your point here :-))
Yeah, no problem for the logo. Sorry for more comments, but some final things on duplicated content: the new website also duplicates now the roadmap and the ecosystem pages from the docs. For those I would do the same as for the governance / contributing docs: leave that for separate PRs, as I don't think we should duplicate content here. |
I think there is agreement to move the roadmap and the ecosystem here, so I'll get rid of the versions in the docs if this is now ready to be merged. |
Where has that been discussed? (I maybe missed some issues .. that's a problem with our huge number of issues) |
There are few issues with discussions about the website, but the most important one was in this PR. If I also remove these pages, this PR contains literally a home page that is incomplete, and not much more. So, I start to miss the point on whether this is a step forward, or we're just wasting time implementing something there is no minimum agreement on. I'll leave this on hold until those decisions are made, working on this for weeks, and reverting everything again doesn't seem to be a great investment of my time, and it's quite frustrating. |
Sorry, Marc, I didn't want to demean your efforts, and I understand your frustration as I have been very slow with feedback. Sorry for your that. For me, there is still a clear added value of this PR: it is providing the base for working on a better home page (which is great work, as this is highly needed!). The exact items in the community dropdown are IMO not essential in the idea of a better website (although I have a strong opinion on it). It was in that light that I thought it would be easier to leave that for later. But I am also happy with merging this as is. It's just some text files that we can also further discuss after they are merged. |
I’m also okay with merging as is and whittling down details later. I think this is a great improvement over current configuration
…Sent from my iPhone
On Sep 17, 2019, at 8:56 AM, Joris Van den Bossche ***@***.***> wrote:
Sorry, Marc, I didn't want to demean your efforts, and I understand your frustration as I have been very slow with feedback. Sorry for your frustration.
For me, there is still a clear added value of this PR: it is providing the base for working on a better home page (which is great work, as this is highly needed). The exact items in the community dropdown are IMO not essential in the idea of a better website (although I have a strong opinion on it). It was in that light that I thought it would be easier to leave that for later. But I am also happy with merging this as is. It's just some text files that we can also further discuss after they are merged.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Either order works for me as well. We'll be fixing things up anyway (since
we're not yet removing the old roadmap.rst, right?)
So merge this and let's iterate on it over the next few weeks?
On Tue, Sep 17, 2019 at 11:16 AM William Ayd <notifications@github.com>
wrote:
… I’m also okay with merging as is and whittling down details later. I think
this is a great improvement over current configuration
Sent from my iPhone
> On Sep 17, 2019, at 8:56 AM, Joris Van den Bossche <
***@***.***> wrote:
>
> Sorry, Marc, I didn't want to demean your efforts, and I understand your
frustration as I have been very slow with feedback. Sorry for your
frustration.
>
> For me, there is still a clear added value of this PR: it is providing
the base for working on a better home page (which is great work, as this is
highly needed). The exact items in the community dropdown are IMO not
essential in the idea of a better website (although I have a strong opinion
on it). It was in that light that I thought it would be easier to leave
that for later. But I am also happy with merging this as is. It's just some
text files that we can also further discuss after they are merged.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#28014?email_source=notifications&email_token=AAKAOISVRYRSTKSB3GWQNK3QKD7HHA5CNFSM4INCLXP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD65CJUA#issuecomment-532292816>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOISSYZM34PUCPB6EUDLQKD7HHANCNFSM4INCLXPQ>
.
|
Yes, fine with merging as is and iterate. To try to explain a bit more the potential reason for some of the disagreements we are having (what I also tried to say at EuroScipy regarding the doc theme / integration with the website): I have been working on a new documentation theme, and for me, the main driver for that is because I find the navigation functionality of the current theme lacking. |
Thanks for clarifying your point, I didn't understand the reasoning. The division that I used was what needs to be served from master, and what from the latest release. I think it simplifies significantly how things are built and deployed. In any case, I think we all agree we need to build the new website in iterations. I think after merging this (I'll do it tomorrow if there are no objections), and making a first integration with the docs, will be easier to see the advantages and disadvantages of every option. @jorisvandenbossche do you think you can have a first version (possibly minimal) of the new docs layout merged soon? So we can experiment using the same templates in both the website and the docs (I think it shouldn't be difficult, and very useful). |
Nice work Marc!
…On Wed, Sep 18, 2019 at 8:25 AM Marc Garcia ***@***.***> wrote:
Merged #28014 <#28014> into
master.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#28014?email_source=notifications&email_token=AAKAOIVSDRIEOQFLK3RADBDQKIT35A5CNFSM4INCLXP2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTWIKTQI#event-2643503553>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIVMKHZE7IVPHBH2HO3QKIT35ANCNFSM4INCLXPQ>
.
|
The website can be seen here: https://datapythonista.github.io/pandas-web/
It's still pending that the designer polish the styles. And also the content can surely be improved with everybody's feedback. But I think will be useful to merge this once we iterate a bit on people's feedback, so we can start working on integrating the docs.
I'm coordinating with the devs of numpy/scipy, scikit-learn and matplotlib to see if we can reshare things implemented here. Ideally I'd like to have the
pysuerga.py
script in a separate repo (that's why it's not named pandas_web.py`), and also the layout and pages that can be shared (the code of conduct, the donate, the blog,...). For now I add everything here for simplicity, and will create a new project and have it as a dependency here if they are interested.CC: @pandas-dev/pandas-core