Browser display cleanup

Add the display of the image

Edit: wrong issue. Too many tabs.

Slight change to the first patch- opens the media browser with smaller vertical height to render the upload form more cleanly as that one is always displayed on open. I've made the browser narrower to force only 5 media items in a row which makes the display cleaner.

There needs to be a height fix when you use the pager on the media view to prevent the scroll bar from briefly appearing.

As per the order change- the libraries are loaded in order so if you want to override some of the default CSS from the jquery libraries it is much easier to have your library load after the others.

Rerolled patch without reference to missing CSS file.

The double quotes are so that \n can be used for line breaks. I can add a . "\n"; if you don't like the escaped code but I do prefer that the resulting markup has line breaks.

Reroll

Makes tonnes of sense to me- less code is less code. Rerolled with just using item_list and a tweak to the css file, theme functions removed.

I should probably address the formatting of the views thumbnails so that they center and don't stretch.

This patch has a few tweaks to how the thumbnails are displayed in the browser. This needs to be checked against the admin view at admin/content/file/thumbnails.

Good point, patch is rerolled with these suggestions.

In looking over some of this code, I think one issue with the CSS could be addressed by changing the dimensions in media_image_default_styles() to reflect what actually gets displayed. We could reduce the complexity of the code by making the display of the images just use the image dimensions rather that using CSS to size the image.

Also it seems like theme_media_admin_thumbnail() is never being called- media_preview_ajax() is the only function that I see that references it but I couldn't find any reference to that function either. Though removing these are probably beyond the scope of this issue they are connected to the display of the thumbnails so it's worth noting.

Rerolled the CSS to not be height/width specific. Note that this will now show all the media thumbnails at the size specified by square_thumbnail. We may want to consider changing the dimensions in the image style.

That last diff does set it 100x100- forgot to set it back after testing. If we do include it at this size we should consider running an image style flush. It is somewhat of an issue because somebody may have used this image style elsewhere. We could also write an update function that has code similar to media_update_7206 to create a new style. I'm not sure of what the best route to go is here.

Even if the image style has be reused elsewhere the admin could just go and reset the configuration and flush the image style again. We could make a upgrade note about this and leave it at that. Though it'd be nice to rename "square_thumbnail" to "media_preview" or something that is a bit more specific for what it is used for.

Here's a reroll with an update to flush the old square_thumbnail and changing the db name to media_thumbnail if it is in the db because it has been overridden.

Woops- here's a reroll with the name change in the install function.

Ugh. Reroll

@aaron- does the markup itself show the size this way? Does media_thumbnail show up in the list of image styles and is it set to 100x100?

removed

UGH wrong issue, please excuse

@aaron so I think this was because your display style had the square_image in its setting. I'm not clear as to why mine didn't- I tested a clean install and an upgraded install- but I think this new patch fixes it.

The issue now is that non-image content in the browser does not have height and width settings. My approach is to look at elements that are not using the image_style theme function, extract the current height/width settings for the media_thumbnail image style and try to apply those to the non-image content. I'm also trying to use the icons that are provided with media to render those instead of the core icons. This is a bit ugly, but it provides a better UX I think. We'd probably want to make an icon for pdfs or figure out a nicer way to decide what file we want to display. My code is really proof of concept.

This patch takes the changes from #34 and takes the patch from #1743040: "You have not selected anything!" error message, when a selection has clearly been made. The main difference here is a significant refactor of theme_media_thumbnail().

I defer to others on this but it seems to me like zoom functionality (while awesome!) is a feature maybe beyond the core functionality of media. A module implementing zoom functionality could use hook_image_styles_alter() to change the default size and then apply the JS.

If people feel otherwise we should probably then increase the dimensions and set the js slide code to start at a lower res. Alternatively, we could provide a second style with larger dimensions and add an image_style url as a data attribute to the thumbnail. When the zoom action starts we can swap the the src value on the image with the larger image style.

I cleaned up this patch a bit. I'm still not thrilled about how theme_media_thumbnail() is working but I think it's cleaner than it was. I've moved the resizing code into theme_media_formatter_large_icon() which will then match the sizing that media_thumbnail specifies. Maybe this is overkill at this point- at worst this patch improves the markup and label handling in theme_media_thumbnail()

Reroll against head

@gmclelland - yes- clicking the tabs does make them shift- the active tab's text wasn't bold. Attached patch fixes that. As per other themes, wow, looks like there is some work to do.

I've been messing around with support for multiple themes and I think this is a larger issue than I'd hoped. My original approach was simply adding some classes to the tabs so that they render out the same way the theme does. This works for some themes, though not for bartik because the jquey.ui.tabs.css adds css which ends up overriding/altering the display of the core theme styles. I don't really see a way around this except for only including the the jquery.ui.tabs.min.js and not the css. Doing this improves the situation but the theming still isn't quite right because the structure of the elements is different than what the theme expects.

So I tried to get around this by trying to emulate the display of theme_menu_local_tasks() to render the tabs which definitely improves things but it is likely the case that unless we really try to create the output of the media browser to approximate the standard page elements it's not going to fare well cross theme.

I see three approaches to move forward:
* only officially support the seven admin theme, continue our semi-integrated/semi-custom approach
* try to replace as many elements in the media browser with standard drupal elements and override the jquery.ui.tabs.css or get rid of it all together
* abandon jquery tabs all together and use standard drupal page elements with custom JS to handle the tabs. This actually turns out to be quite simple to do- I added some data attributes to the existing markup and wrote this JS as a proof of concept.

      // Ensure that there is an active pane when the browser opens.
      if (! $('.media-browser-pane .active').length) {
        $('.media-browser-pane').first().addClass('active');
      }

      // Hide all the non-active panes on page load.
      $('.media-browser-pane').not('.active').hide();

      // Set the correct tab to active based on the currently active pane.
      var tabid = $('.media-browser-pane.active').attr('data-tab-id');
      $('.media-browser.tab[data-tabid=' + tabid + ']').addClass('active');

      // Switch tabs with clicks.
      $('a.media-browser-tab').bind('click', function () {
        // Ignore clicks on already active tabs.
        if (! $(this).hasClass('active')) {
          // Make all tabs inactive.
          $('a.media-browser-tab.active').removeClass('active');
          // Make this tab active.
          $(this).addClass('active');
          // Make all panes inactive
          $('.media-browser-pane.active').removeClass('active').hide();
          // Activate the correct pane.
          var tabid = $(this).attr('data-tabid');
          $('.media-browser-pane[data-tabid=' + tabid + ']').addClass('active').show();
        }
        return false;
      });

There would need to be some cleanup in terms of error handling but it does work. I'm not sure if it is a viable way forward, I'm just trying to figure out how to improve the sustainability of display inside the browser.

Thoughts?

Unfortunately option 1 doesn't really work with all themes because of different kinds of assumptions about markup. I'm going to take another shot at rendering the tabs through media_local_tasks and overriding the jquery.ui css where I can. It is also worth considering removing some of the classes when the tabs are triggered.

I've done a bunch of cleanup here that I think is a starting point for improving things further. The stuff in media_browser() adds a degree of complication but I was trying to make it easier to get things into Drupal elements. I'm still privileging the Seven theme but Bartik is no longer terrible.

I think it might be worth considering breaking some of this work out into templates so that it is easier for other themes to override- that maybe the simplest solution for theme maintainers.

Ok, here's another crack at this. Moves some of the display code into the browser template, cleans up some JS. Spruces up the display type screen as well.

Here is a slight cleanup of the above.

@jgahunia I'm unclear on what in particular is broken for media front here? Is it that the preview for the media front item is constrained by the width of the preview size? While I really like that media front is providing a preview, the intent of the display in the media browser is thumbnails- perhaps a opening a modal with a preview would be a better bet?

Fixed broken image link