Skip to content

Conversation

martinbean
Copy link

Added responsive styles, such as making the menu collapsible and content columns fluid and collapsible at small widths.

@zpao
Copy link
Member

zpao commented May 30, 2013

Thanks Martin! I'll take a look at this soon, we've just been following up with some other issues today. Not forgotten though :)

width: auto;
}
.nav-docs {
display: none;
Copy link
Member

Choose a reason for hiding this comment

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

This is the only behavior that's bugging me. Since this nav is important for getting around the docs, hiding it is a bummer. Can you do something else with it instead? (Maybe unfloat, and compact it a bit?)

@martinbean
Copy link
Author

Hi Paul, I’ll get on these two issues now.

function classToggle(element, tclass) {
var classes = element.className;
var pattern = new RegExp(tclass);
var hasClass = pattern.test(classes);
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a pretty naive implementation (eg. if the element had class="reallyopen" it would not work right).

var classes = ' ' + element.className + ' ';
var hasClass = classes.indexOf(' ' + className + ' ') > -1
// then add/remove, trim, set

Copy link
Author

Choose a reason for hiding this comment

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

I see 😞

Copy link
Member

Choose a reason for hiding this comment

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

If you're attached to RegExps, you might be able to just use '\b' + tclass + '\b' when constructing so it matches word boundaries.

The other thing we should do is actually set up listeners instead of hijacking window.onload. We do this in our transform script so we should do that: https://github.com/facebook/react/blob/master/vendor/browser-transforms.js#L106-L110

Copy link
Author

Choose a reason for hiding this comment

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

I’m not particularly attached to RegExp; in fact JavaScript isn’t my main proficiency as you may see above! That class toggle function was just something I cobbled together without the comfort of jQuery.

I’ve pushed the style changes mentioned, and will look at improving the JavaScript for the responsive menu.

Choose a reason for hiding this comment

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

You can't use word boundaries (\b) for this since hyphens are valid in class names. You need something like new RegExp('(?:^|\\s)' + tclass + '(?:\\s|$)').

@zpao
Copy link
Member

zpao commented Jul 17, 2013

Hey @martinbean, I'm sorry we haven't made this work. We're actually working a on sizable redesign and I think it's going to be response from the start there (yea @jordwalke?), so I'm going to close this out and let it just happen there. I really appreciate you taking the time to give this a go though!

@zpao zpao closed this Jul 17, 2013
matthewjohnston4 added a commit that referenced this pull request Mar 3, 2015
vjeux added a commit that referenced this pull request Mar 4, 2015
matthewjohnston4 added a commit that referenced this pull request Mar 4, 2015
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