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

Document use of Image List with a LTR Masonry solution; consider renaming classes/mixins #2865

Closed
dessant opened this issue Jun 2, 2018 · 11 comments
Labels

Comments

@dessant
Copy link

dessant commented Jun 2, 2018

It appears the images are laid out vertically, from top to bottom. This may not be the most common or expected stacking order, and the design guidelines seem to imply that the order is ltr or rtl.

https://material.io/design/components/image-lists.html#types

@theenoahmason
Copy link

theenoahmason commented Jun 4, 2018

Agreed. This is not a masonry layout, it is a traditional newspaper column based layout. CSS columns work much better for textual content than for displaying lists of this kind.

@kfranqueiro
Copy link
Contributor

Thanks for raising this issue.

The design guidelines don't seem to explicitly indicate one way or the other, but I talked to our designer on the image list component and she said that the implication was indeed supposed to be LTR or RTL.

The advantage of the current approach is that MDC Image List requires only minimal CSS, with no JS, and serves its purpose in cases where it occupies a set region and the order of images doesn't matter. Additionally, there are already existing, mature, standalone JavaScript implementations for LTR masonry, so creating our own JS implementation seemed like it would be misplaced effort.

I could see potentially investigating the idea of documenting how to integrate a third-party JS masonry implementation to adhere to Material's style guidelines, in addition to providing our CSS-only solution. Does that sound reasonable?

@dessant
Copy link
Author

dessant commented Jun 5, 2018

Regarding the CSS-only solution, I believe components in this library should either adhere to the current Material Design spec, or they should be removed.

@williamernest
Copy link
Contributor

@dessant Can you point out where you saw that in the spec? I've gone over the spec a couple of times and can't find anything indicating the direction.

@dessant
Copy link
Author

dessant commented Jun 5, 2018

I agree that there is no need to rewrite an existing masonry implementation, though an existing library could be imported and set up according to the spec, and provided as a component with the usual features one expects to find in MDC components.

@dessant
Copy link
Author

dessant commented Jun 5, 2018

@williamernest it's not stated explicitly in the spec, but horizontal ordering is considered to be the correct implementation by the designer of this component, so it's a matter of mentioning this in the guidelines to avoid these implementation mistakes.

@theenoahmason
Copy link

theenoahmason commented Jun 5, 2018

Masonry layouts and column layouts are not the same thing. They are night and day. You dont look at a newspaper and say, "Wow those are some sweet masonry paragraphs." That is because they are in a traditional column based layout. Columns are not Masonry.

Masonry MEANS horizontal, as it should pack a grid of ltr elements reguardless of their heights, their order should be retained as much as possible (although the packing may require reordering, like in Metafizzy's Masonry). Columns MEAN vertical.

I suggest calling modifier "--columns" instead of "--masonry," or remove this modifier alltogether.

In my opinion the typography component should recieve the CSS column treatment, it would be nice to use columns for what they are meant for.

I have used MDC web with Masonry, Isotope, and Packery (all by metafizzy/desandro) and they work perfectly fine.

@kfranqueiro
Copy link
Contributor

I'm going to track this RE including guidance (and potentially tailoring the styles we provide) for use with a third-party masonry library, probably Masonry.

RE renaming masonry to columns, we'd have to figure out something that makes sense in the context of the mixin to set the number of columns as well - mdc-image-list-columns-columns (vs. the current mdc-image-list-masonry-columns) would sound confusing.

Thanks for the feedback, everyone.

@kfranqueiro kfranqueiro changed the title Masonry image list is laid out vertically Document use of Image List with a LTR Masonry solution; consider renaming classes/mixins Jun 7, 2018
@theenoahmason
Copy link

theenoahmason commented Jun 7, 2018

Using MDC Web Image Lists with Masonry

Working reduced test case on codepen:
https://codepen.io/theenoahmason/pen/NzbqjB

HTML

<ul class="mdc-image-list mdc-image-list--masonry-js">
	<li class="mdc-image-list__item">
		<img 
		class="mdc-image-list__image" 
		src="..."
		/>
		<div class="mdc-image-list__supporting">
			<span class="mdc-image-list__label">...</span>
		</div>
	</li>
	...
</div>

Sass

.mdc-image-list { 
	&--masonry-js &__item {
		// width: calc((100% / $column-count) - (($gutter-size * ($column-count - 1)) / $column-count));
		width: calc((100% / 6) - ((16px * 5) / 6));
		// margin-bottom: $gutter-size;
		margin: 0 0 16px;
	}
}

JavaScript

const imageListElement = document.querySelector('.mdc-image-list--masonry-js');
// init on imagesLoaded to ensure calculations are done properly. 
imagesLoaded(imageListElement, () => {
	new Masonry(imageListElement, {
		// use the list item selector as the column width.
		columnWidth: '.mdc-image-list__item',
		// only apply masonry to the list item selector.
		itemSelector: '.mdc-image-list__item', 
		// ensure widths are calculated by percentages.
		percentPosition: true, 
		// ensure this is the same as the sass gutter. 
		// this could also always be set by a `data-masonry-gutter` attribute, or by getting the computed `margin-bottom` of the first list item.
		gutter: 16, 
	});
});

Notes

  • Initializes Masonry on imagesLoaded (also by metafizzy/desandro) to ensure calculations are correct.
  • Provides a new calculation of column sizes (not that dissimilar to the image-list-standard-columns mixin calculation).
  • The gutter in Sass should correspond exactly to the gutter provided to the Masonry script.
  • The margin-bottom of .mdc-image-list__item should match the gutter.
  • Gutter is at 16px to match spec.
  • Currently being applied to the non-existent modifier of mdc-image-list--masonry-js because the existing mdc-image-list--masonry modifier uses CSS column layout (not suitable for horizontal masonry layout).
  • I have only used those Masonry options that are absolutely necessary, see more here.

Refinements

Preserving exact horizontal order: Using horizontalOrder: true will enforce the horizontal order, though It is less good at packing this way, and will leave longer danglers at the bottom of the list in most cases. When this is off, Masonry looks for the closest slot to the top.

Hide untill ready: Using the layoutComplete event could allow for showing the list only when imagesLoaded and Masonry is complete; presumably by an -upgraded class.

const MasonryInstance = new Masonry(...);
// bind event listener to be triggered just once
MasonryInstance.once('layoutComplete', (e) => {
  ...
});

@kfranqueiro I have made an attempt to make this pretty thorough, but please let me know if you want me to provide any more information.

@theenoahmason
Copy link

@kfranqueiro It's also worth noting here that CSS Column layout as used in masonry image lists as it currently stands will only work if the tiles are in multiples of the column count. This ensures that there are never horizontal rows with uneven amount of tiles.

This is simply not feasible to use as is IMO.

@allan-chen
Copy link
Contributor

Closing in favor of newer #6548 for tracking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants