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

Responsive guides #6475

Merged
merged 2 commits into from Oct 7, 2012
Merged

Responsive guides #6475

merged 2 commits into from Oct 7, 2012

Conversation

joefiorini
Copy link
Contributor

I started implementing a responsive design for the Rails guides. I've only tested it on Apple laptops & portable devices, but I wanted to get feedback before going much further. What further do we need to do to pull this in and get it deployed?

(Note: I originally requested this on lifo/docrails and was asked to send it here instead)

@fxn
Copy link
Member

fxn commented May 24, 2012

Awesome, let me play with this a little bit locally before applying.

@drogus
Copy link
Member

drogus commented May 24, 2012

@joefiorini Isn't it better to use jquery instead of zepto, to support most of mobile browsers?

@joefiorini
Copy link
Contributor Author

Yes, I think you're right. My intention was to switch that. I'll push an update tonight.

On May 24, 2012, at 4:17 PM, Piotr Sarnackireply@reply.github.com wrote:

@joefiorini Isn't it better to use jquery instead of zepto, to support most of mobile browsers?


Reply to this email directly or view it on GitHub:
#6475 (comment)

@joefiorini
Copy link
Contributor Author

jQuery is now in. Thanks for asking.

@joefiorini
Copy link
Contributor Author

To make it easier for people to try it out, I just published my fork of the guides. This is certainly not intended to be anything more than a way for others (and myself) to more easily try to responsive design on different devices. If you're interested check it out http://rails-guides.joefiorini.com.

@drogus
Copy link
Member

drogus commented May 26, 2012

@joefiorini that looks really cool! :D

@agis
Copy link
Contributor

agis commented May 26, 2012

Cool!

@goshacmd
Copy link
Contributor

Great!

@steveklabnik
Copy link
Member

So cool.

@jherdman
Copy link

That's awesome, man! Thanks!

@roytomeij
Copy link
Contributor

👍

@dominic
Copy link

dominic commented May 26, 2012

It'd be nice if the navigation and "Guides Index" link broke to the mobile layout w/ the drop-down menu earlier, as it's not usable at "medium" screen widths.

@vijaydev
Copy link
Member

  • Squash all commits into one
  • Use minified jQuery

@@ -856,7 +856,7 @@ SELECT categories.* FROM categories
INNER JOIN posts ON posts.category_id = categories.id
</sql>

Or, in English: "return a Category object for all categories with posts". Note that you will see duplicate categories if more than one post has the same category. If you want unique categories, you can use Category.joins(:posts).select("distinct(categories.id)").
Or, in English: "return a Category object for all categories with posts". Note that you will see duplicate categories if more than one post has the same category. If you want unique categories, you can use @Category.joins(:posts).select("distinct(categories.id)")@.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change part of this PR? Can you make this change in docrails and remove it here?

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 needed to put a tag around it so I could control the text wrapping from CSS to make it fit nicely on a small screen. I would consider this part of the responsive design, not necessarily just a content change.

Copy link
Member

Choose a reason for hiding this comment

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

We usually recommend code snippets to be formatted in the guides and api docs. That's the reason I asked for the move. But I'm not necessarily bothered by this being here.

@joefiorini
Copy link
Contributor Author

@vijaydev I ended up squashing it into two commits. Is that good enough?

@vijaydev
Copy link
Member

@joefiorini Squash looks good.

@joefiorini
Copy link
Contributor Author

Thanks @vijaydev! Is there anything else I need to do to get this merged in?

@joefiorini
Copy link
Contributor Author

Just made a couple changes:

  1. Styled the Guides Index drop down; I think it looks much better
  2. Made the floating right column drop down under the main content on tablets and medium sized screens

I also plan on adding a medium-display style for the navigation and improving the small-display style.

@morgoth
Copy link
Member

morgoth commented Jul 21, 2012

+1 Example page looks nice: http://rails-guides.joefiorini.com

@fxn
Copy link
Member

fxn commented Jul 21, 2012

@morgoth thanks for the feedback, let's aim at merging this in the following days.

@joefiorini I have seen in the AS guide code snippets of different sizes in the iPhone, do you know why? Also the guides select says "Active Support ..." instead of "Guides Index", which is what the regular and iPad versions show. Is that intentional?

@ghost ghost assigned fxn Jul 21, 2012
@joefiorini
Copy link
Contributor Author

Are you referring to the I18N table? I still need to go through each page and fix instances where pre tags and tables are forcing the width beyond the bounds of the viewport. This causes unwanted horizontal scrolling.

As for the Guides Index I set it up to select whichever guide you are currently viewing. I'll change this back to match the behavior of the other formats.

Sent from my mobile device

On Jul 21, 2012, at 7:09 PM, Xavier Noriareply@reply.github.com wrote:

@morgoth thanks for the feedback, let's aim at merging this in the following days.

@joefiorini I have seen in the AS guide code snippets of different sizes in the iPhone, do you know why? Also the guides select says "Active Support ..." instead of "Guides Index", which is what the regular and iPad versions show. Is that intentional?


Reply to this email directly or view it on GitHub:
#6475 (comment)

@joefiorini
Copy link
Contributor Author

Nevermind, I see what you are talking about on the AS guide. I'll look into that when I'm on my computer again.

On Jul 21, 2012, at 7:09 PM, Xavier Noriareply@reply.github.com wrote:

@morgoth thanks for the feedback, let's aim at merging this in the following days.

@joefiorini I have seen in the AS guide code snippets of different sizes in the iPhone, do you know why? Also the guides select says "Active Support ..." instead of "Guides Index", which is what the regular and iPad versions show. Is that intentional?


Reply to this email directly or view it on GitHub:
#6475 (comment)

@carlosantoniodasilva
Copy link
Member

Hey @joefiorini, are we ok with this one, to merge it, or is there anything else missing? Thanks.

@joefiorini
Copy link
Contributor Author

@carlosantoniodasilva I still need to make the fix that @fxn recommended. I'll get that in today, and I think we'll be ready to merge then.

@carlosantoniodasilva
Copy link
Member

Alright, no hurries, just wanted to make sure it was not going to die :). Thanks.

@fxn
Copy link
Member

fxn commented Aug 31, 2012

@joefiorini just a ping... any news?

@schneems
Copy link
Member

schneems commented Oct 4, 2012

@joefiorini this work is great, love the example page thanks for working on this! Will you be able to make those changes and update this PR soon?

@joefiorini
Copy link
Contributor Author

I'm not sure I will, but it's actually a fairly small bug on iOS in tables.
Could we go ahead and get this out there and I can open an issue for that
bug?

On Thu, Oct 4, 2012 at 4:37 PM, Richard Schneeman
notifications@github.comwrote:

@joefiorini https://github.com/joefiorini this work is great, love the
example page thanks for working on this! Will you be able to make those
changes and update this PR soon?


Reply to this email directly or view it on GitHubhttps://github.com//pull/6475#issuecomment-9156233.

@steveklabnik
Copy link
Member

I'd say if it's just a small bug, :shipit:

This will still need a rebase, though.

@joefiorini
Copy link
Contributor Author

Rebased, should be ready to go. Can someone merge this in? I'm going to test the small bug I mentioned on iOS 6, if it's still there I'll open an issue for it. Should that be in rails or docrails?

@fxn
Copy link
Member

fxn commented Oct 6, 2012

@joefiorini following the thread, I am not sure what is pending as an iOS bug from my two remarks. Which remark is fixed and which isn't? Is http://rails-guides.joefiorini.com up to date?

@joefiorini
Copy link
Contributor Author

Actually, nothing anymore. Looks like iOS 6 fixed whatever the problem goes. I think it's good for a first release.

On Oct 6, 2012, at 5:49 PM, Xavier Noria notifications@github.com wrote:

@joefiorini following the thread, I am not sure what is pending as an iOS bug from my two remarks. Which remark is fixed and which isn't? Is http://rails-guides.joefiorini.com up to date?


Reply to this email directly or view it on GitHub.

@steveklabnik
Copy link
Member

Awesome. :shipit:

I'd merge this if I was allowed.

@rafaelfranca
Copy link
Member

:shipit:

@fxn
Copy link
Member

fxn commented Oct 6, 2012

@joefiorini but the label is revised?

@joefiorini
Copy link
Contributor Author

You mean your comments from 7/21? Those should be fixed now. Sorry for the confusion.

@joefiorini
Copy link
Contributor Author

I was referring to an issue with the responsive tables I implemented to fix one of the problems you found. It doesn't appear to be an issue anymore in iOS 6.

@fxn
Copy link
Member

fxn commented Oct 7, 2012

Excellent, thanks a lot for this work!

fxn added a commit that referenced this pull request Oct 7, 2012
@fxn fxn merged commit 93d2251 into rails:master Oct 7, 2012
yhirano55 added a commit to yhirano55/rails that referenced this pull request Mar 31, 2018
* .clearfix is overridden.
* .clear is not currently used.
* #extraCol is not currently used.
* table th, table td are overridden. Merged them.
* Removed needless comment lines which are added on rails#6475
@yhirano55 yhirano55 mentioned this pull request Mar 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet