Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markhalliwell’s picture

markhalliwell’s picture

tr33m4n’s picture

I like the idea of using the panels styling as fieldsets (if that's what this is implying). Was thinking of doing this myself on a project I was working on... Will see if I can spare time to put in a patch

tr33m4n’s picture

Preliminary patch that adds classes to fieldset. Need to figure out the best way of adding collapsible js and doesn't seem right removing <label>, probably would be best to override the bootstrap styles instead

tr33m4n’s picture

It does look tidier however with the panel styles

markhalliwell’s picture

Status: Active » Needs review
FileSize
5.07 KB
5.68 KB

Ok here's my attempt at utilizing existing JS that is loaded from Bootstrap (collapsible).

markhalliwell’s picture

Hmm, let's try this again without removing a previous commit

markhalliwell’s picture

FileSize
50.85 KB

Screenshot:
bootstrap_panels.jpg

markhalliwell’s picture

I put some @todos in for figuring out vertical_tabs (which are overridden by this patch). I created a follow-up issue to deal with them: #2094409: Figure out how to theme vertical_tabs properly.

tr33m4n’s picture

Appear to be getting this error on the login page

Notice: Undefined index: #title in bootstrap_preprocess_bootstrap_panel() (line 251 of /sites/all/themes/bootstrap/includes/form.inc)

markhalliwell’s picture

I fixed the error in #10. I also added some touch-ups to some CSS/template file to make the entire panel header (a tag) clickable. Put in some JS to prevent the default click on a.collapsible elements so the #hash links (for no-js fallback) don't actually move the page around if JS is enabled. If someone can review and RTBC this, I'd like to get this in asap.

markhalliwell’s picture

Status: Needs review » Fixed

I've decided to just push this (to keep momentum going). If any regressions are found, open a new issue.

Committed a28e08a to 7.x-3.x.

tr33m4n’s picture

Testing at the moment, no problems as of yet

tr33m4n’s picture

Although this change is visually better, I do think that removing the <label> element from the fieldset is a bad idea in terms of accessibility, screen readers etc. Hence my previous hesitations and questions about whether this could be a css styling rather than a complete rework

markhalliwell’s picture

I'd rather just keep to what BS examples show (for now). I really don't think accessibility is an issue here.

tr33m4n’s picture

Ok cool :)

markhalliwell’s picture

Version: 7.x-3.x-dev » 7.x-3.0-rc1

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.