Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

site-wide social include #12102

Closed
wants to merge 25 commits into from

Conversation

aaronmarkham
Copy link
Contributor

@aaronmarkham aaronmarkham commented Aug 9, 2018

Features

Social logos appear on the upper right. They move orientation as the screen gets smaller. Then disappear when it goes into mobile view. The hamburger menu now has a text version of the social options. I also threw the forum in that list.

There was some duplication and discrepancies with desktop, so I cleaned those up a bit.

In my journey I found some bugs and deprecations so those were addressed too.

Preview

Leaning towards this one (plus it has layout fixes):

Comments

Long story short, the site is kind of stuck with old bootstrap, old jquery, and quite a mess of display issues due some hackery. So things don't work quite as they should. I tried updating bootstrap and jquery, but these ended up being incompatible with the site due to said hackery and would need a lot of work.

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include

fixup! trying out site-wide social include
@aaronmarkham aaronmarkham requested a review from szha as a code owner August 9, 2018 03:45
@nswamy
Copy link
Member

nswamy commented Aug 9, 2018

played around a bit on the link you provided, 2 observations

  1. why do we need on all pages? I think its sufficient to just have it in main page and tutorial pages, everywhere else I feel its a distraction.
  2. Logos are of very poor quality and on my 1080p monitor they look pretty bad, don't they have high def logos?

Copy link
Contributor

@lupesko lupesko left a comment

Choose a reason for hiding this comment

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

Awesome work!

See a comment below, plus:
Logos are low res, render bad on hi-rez screens.

@@ -62,13 +62,37 @@ To join the MXNet slack channel send request to the contributor mailing list.

### Social Media

Keep connected with the latest MXNet news and updates on [Twitter](https://twitter.com/apachemxnet) and [Reddit](https://reddit.com/r/mxnet). Also, subscribe to the [MXNet YouTube channel](https://www.youtube.com/channel/UCQua2ZAkbr_Shsgfk1LCy6A).
Keep connected with the latest MXNet news and updates.
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole section renders bad, with the logos not aligned properly. Needs some work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started with the community page and hated how ugly that social section was. That's why I added the nav bar. I'll circle back and fix it.

Also I can add higher resolution images. I might need to requestion a 4k monitor 😋

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool.

I might need to requestion a 4k monitor 😋

I'm sure you can find a manager at AWS that will approve such a request...

@aaronmarkham
Copy link
Contributor Author

@swamy for the tutorial pages I had something else in mind. I would provide a different layout.
It would stack like...
Nav
Title
Abstract
Prerequisites
Tags
Share this page buttons
Video if available
Content
CTAs:

  • Related tutorials links
  • Share this page buttons

With this PR I was testing a simple concept that uses Sphinx's templating system and the site's current layout as a precursor.

SK's advise was to redo the site to fix all of the bootstrap and Sphinx hacks so it is easy to update and will behave as expected. I won't attempt anything fancy after what I just went through to get even this working.

I can turn off the include for certain sections. I can add the text menu to the nav to align it with mobile. I can bump the res of the images. After that, it's pretty hard right now.

Also, I think the buttons should call some attention. We want people to click on them. I'd like to see more votes on limiting which pages they appear.

@nswamy
Copy link
Member

nswamy commented Aug 9, 2018

@aaronmarkham feel free to raise this discussion on the dev@ and seek more opinions. At the moment I feel the design needs a little more work and is not cohesive with the current website layout.

@aaronmarkham
Copy link
Contributor Author

I'm using svg images instead now, so things should look good an whatever resolution, plus they're small. And yes, I put them in the mxnet repo because otherwise they wouldn't work without switching to embedding and tl;dr: no thanks.

I also posted the images and associated Sketch files to the web-data repo in case anyone (like a designer!) wants to tinker with improving the images.

The contribute page looks better now... and also uses the svg images.

nswamy
nswamy previously requested changes Aug 9, 2018
Copy link
Member

@nswamy nswamy left a comment

Choose a reason for hiding this comment

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

The logos look Ok, the responsive design is broken. Still don't like it on every page, just because you put on every page doesn't mean people will click on it. I hate such distractions when I go to read documentation, please raise it on dev@ to find If i am the only exception.

screen shot 2018-08-09 at 9 57 35 am

screen shot 2018-08-09 at 9 53 45 am

@nswamy nswamy added Website pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Aug 9, 2018
@aaronmarkham
Copy link
Contributor Author

@nswamy I fixed the responsive design for your higher res monitor. Though I'm sure the css could use an overhaul.

@lebeg
Copy link
Contributor

lebeg commented Aug 15, 2018

I tested on my Mac / Chrome / 34-inch (3440 x 1440) and the social icons don't look good.
screen shot 2018-08-15 at 11 46 47
I my opinion the icons are distracting a bit. But the main problem is the complexity of their layout. Do you think it would make sense to try them on the left menu instead? The one that gets hidden on tight mobile sceens.

@aaronmarkham
Copy link
Contributor Author

@lebeg Take a look at the preview on your mobile or just resize the screen to pretty small - the social links are text in that hamburger menu, but only for "mobile", and that view doesn't trigger until under 600px. So then desktop would never see them. I could just add text to the desktop nav instead of buttons, and the CSS complexity goes away.

The current CSS and implementation of boostrap appears pretty messed up. It behaves inappropriately, and I found that the entanglements are huge. Various JS scripts are injecting or overwriting stuff after boostrap has rendered which could explain why bootstrap doesn't work as expected. Like there's a footer bug that me, Krishnan, or SK couldn't figure out. Maybe someone else can, but it seems like it is basically a start-over job with the design, or hack on what we have.

I can tinker with the CSS a bit more and try to get rid of that crowding you see - make buttons smaller increase padding and so forth, but I'm going to have to find a better monitor as what I have does get that high of resolution.

I like incremental improvement. If anyone else wants to take a stab at the same feature, that's cool. Or fix/upgrade the bootstrap implementation. That would be awesome.

I just think we really should improve the marketing game on the site and try to boost MXNet's social numbers, and I don't think waiting for a site redesign is ideal.

@simoncorstonoliver
Copy link
Contributor

I agree that it's probably enough to put it on the landing page only.

Note that we have a friendly URL for Youtube now: https://www.youtube.com/apachemxnet

@aaronmarkham
Copy link
Contributor Author

@simoncorstonoliver Thanks for your feedback. I only include the buttons on the home page now. I've updated the Youtube link.

@nswamy
Copy link
Member

nswamy commented Aug 23, 2018

could you show a variation using the logos provided by the platforms ?

@aaronmarkham
Copy link
Contributor Author

@nswamy This one uses the logos from the platforms rather than trying to conform them to the mxnet blue background.
http://34.201.8.176/versions/social_media_update_v2/

@lebeg
Copy link
Contributor

lebeg commented Aug 24, 2018

screen shot 2018-08-24 at 17 39 04
Maybe adding some paddings and reducing the size of the buttons might help improving their appearance.

@lebeg
Copy link
Contributor

lebeg commented Aug 29, 2018

screen shot 2018-08-29 at 10 55 43
Looks good!

@nswamy
Copy link
Member

nswamy commented Aug 29, 2018

Please check the site on different resolutions, on my mac the logos look small

@aaronmarkham
Copy link
Contributor Author

@lupesko - do you still request changes? The latest is hosted here: http://34.201.8.176/versions/social_media_update_v2/

@lupesko
Copy link
Contributor

lupesko commented Sep 18, 2018

@aaronmarkham - looks good, approved.

@lupesko
Copy link
Contributor

lupesko commented Sep 18, 2018

@nswamy res looks good now, kindly review and approve. Thanks!

@vandanavk
Copy link
Contributor

@nswamy ping for review/approval

@aaronmarkham
Copy link
Contributor Author

Since ApacheCon is almost over, I should remove that logo, and rebase all of this to make sure it's all working as expected. That would take care of both things, but I don't want to do that if there's not a consensus on this feature.

@vrakesh
Copy link
Contributor

vrakesh commented Oct 9, 2018

@nswamy requesting a review/approval for this PR.

@Roshrini
Copy link
Member

@aaronmarkham Can you please resolve conflicts so that we can move forward with this PR?

@aaronmarkham
Copy link
Contributor Author

@Roshrini Ok, resolved conflicts... let's see if it clears CI.

@aaronmarkham
Copy link
Contributor Author

aaronmarkham commented Oct 19, 2018

Travis just timed out. So that part is fine, but I'm seeing the CI-provided preview has broken images, so I'll take a look at that.
UPDATE - changing this link to match the latest run...
http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-12102/25/index.html

@ankkhedia
Copy link
Contributor

@aaronmarkham Were you able to work on the broken images?
If that part is fixed we can add reviewers to go forward.

@aaronmarkham
Copy link
Contributor Author

@ankkhedia Yes, the preview link is updated and the images are all there.

@ankkhedia
Copy link
Contributor

@nswamy ping for reviewing again as all the comments has been addressed.

@kalyc
Copy link
Contributor

kalyc commented Nov 12, 2018

@aaronmarkham thanks for your contribution! I see the CI build is failing - could you re-trigger the build with an empty commit?

@stu1130
Copy link
Contributor

stu1130 commented Nov 21, 2018

@aaronmarkham ping again

@vandanavk
Copy link
Contributor

@mxnet-label-bot update [Website, pr-awaiting-testing]

@marcoabreu marcoabreu added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Nov 28, 2018
Copy link
Member

@roywei roywei left a comment

Choose a reason for hiding this comment

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

@aaronmarkham Looks cool! ping to trigger CI

@aaronmarkham
Copy link
Contributor Author

Closing this PR. I can try another round of adding social buttons later. The other updates on the community page are already in #13705.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-testing PR is reviewed and waiting CI build and test Website
Projects
None yet
Development

Successfully merging this pull request may close these issues.