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

Added some additional classes to record tabs #1158

Merged
merged 10 commits into from Apr 20, 2018

Conversation

Projects
None yet
3 participants
@jlcooper
Copy link
Contributor

commented Apr 6, 2018

To make it a bit easier to style the record tab navigation in CSS I've added some classes to them.

Here's an example of reordering tab navigation headings to move the description entry to appear first:

div.record-tabs ul.nav-tabs {
    display: flex;
    
}

li.record-tab {
    flex: 0 1 auto;
    display: inline-block;
}

li.record-tab a {
    height: 100%;
}

li.record-tab.description {
    order: -1;
}
Jason Cooper Jason Cooper

@demiankatz demiankatz requested a review from crhallberg Apr 6, 2018

@demiankatz

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Thanks, @jlcooper, this looks like a potentially useful bit of class labeling to me, but I'll wait for @crhallberg's input before merging in case he has suggestions or concerns.

@crhallberg

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

Hello, @jlcooper, this is a great idea! I have some feedback for you to consider:

  1. We currently have unique classes on the links inside the tabs which we aren't using in the CSS or the JS as far as I can tell. We should either remove those or move them up to the <li> level to prevent redundancy.
  2. I'm trying to standardize the way classes are named from camel case to the hyphenated style Bootstrap uses, so nav-tab or record-tab instead of navTab.
  3. While I understand it's just an example, I'd prefer to use two classes instead of detecting prefixes. So instead of .nav-tab-description, maybe it should be two classes like .nav-tab.description. That way we can style on .nav-tab instead of [class^="nav-tab-"].
  4. Would record-tab be better or more descriptive than nav-tab?
@jlcooper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

@crhallberg, interestingly I ended up going down the prefix route because of the tab name being used on the links within the <li> tags.

I'll look at removing the class from the link and switch to two classes on the <li> tag, I'll also rename the navTab class to record-tab as it'll make more sense when looking at the CSS.

Jason Cooper Jason Cooper
Switched to using two classes to identify the record tabs and removed…
… the class from the link within the record tab.
@@ -86,7 +87,7 @@
if (!$obj->supportsAjax()) { $tab_classes[] = 'noajax'; }
?>
<li<?=count($tab_classes) > 0 ? ' class="' . implode(' ', $tab_classes) . '"' : ''?>>

This comment has been minimized.

Copy link
@crhallberg

crhallberg Apr 13, 2018

Contributor

Final nitpick: if we're populating the array above, it'll never be empty.

@crhallberg
Copy link
Contributor

left a comment

Thanks for incorporating the feedback! One final tiny thing and I think we're ready to go.

@jlcooper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2018

Good catch @crhallberg, that line's now a lot easier to read.

demiankatz added some commits Apr 16, 2018

@demiankatz

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

@crhallberg, I made a couple minor adjustments designed to improve readability... and then I actually tested the code and discovered that tabs are now completely broken.

The problem here is that the Javascript that makes the tabs work relies on the <a> having a class matching the tab name. Without that, the code can't figure out how to load the tab content into the correct place. I think we have two options:

1.) Put things back the way they were, with the tab name on the <a> instead of the <div>.

2.) Rewrite the Javascript and template to use a data- element to pass through the necessary information, which is probably a more technically appropriate approach.

Thoughts?

@jlcooper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

I've made a first stab at using a data- attribute to identify the record tabs.

@demiankatz, @crhallberg - I haven't really looked into VuFind's JavaScript before, so there's bound to be bits I've missed that'll need clearing up.

demiankatz added some commits Apr 16, 2018

@demiankatz

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

@jlcooper, I just made a couple minor adjustments:

1.) I added the $tabName back as a class on the <li> in addition to the data-tab attribute. I thought this would make styling easier, even though it's a little redundant.

2.) One more adjustment had to be made to the Javascript so that the tab name in the URL hash would be properly applied when a page was bookmarked. (The test suite caught this one). Fortunately, it was easy to fix!

I think we're ready to merge now, but I'll wait and see if you or @crhallberg have anything to say before I finalize the PR. Thanks again!

@jlcooper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

@demiankatz, I'm happy with the changes and I've given it a quick test and it's working for my test data.

@crhallberg

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

I'm happy with the updated JS and the use of data-tab. 👍

@vufind-org vufind-org deleted a comment from jlcooper Apr 20, 2018

@demiankatz demiankatz merged commit 1092136 into vufind-org:master Apr 20, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No dependency changes
Details
@demiankatz

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

Thanks for the review, @crhallberg. Just merged!

@jlcooper, I moved your revised CSS example out of your comment and into the main PR description, just so that everything is up to date if people look at this for reference in the future.

@jlcooper jlcooper deleted the jlcooper:recordTabClasses branch Apr 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.