etsy / AndroidStaggeredGrid

An Android staggered grid view which supports multiple columns with rows of varying sizes.
https://github.com/etsy/AndroidStaggeredGrid
4.76k stars 1.13k forks source link

Column count should depend on screen width, rather than orientation #148

Open stanch opened 10 years ago

stanch commented 10 years ago

I have read #9 and #32. IMHO, width being greater or less than height (i.e. landscape or portrait respectively) has nothing to do with the optimum number of columns, since the grid is bounded horizontally, but not vertically. Instead, this value should be based solely on width, e.g.:

[0; 400dp) ⇒ 1 column,
[400dp; 610dp) ⇒ 2 columns,
[610dp; ∞) ⇒ 3 columns

For a more concrete example, I find that my grid looks best:

In XML the corresponding setting could look like this:

app:column_count="0|400|610"

And in code:

grid.setColumnCount(new int[]{0, 400, 610});

Here are the values if you don’t want single columns:

app:column_count="0|0|610"
grid.setColumnCount(new int[]{0, 0, 610});

If you don’t like this proposal, in theory the same effect could be achieved “manually”, using only the current setColumnCount API. However, I would like to note that it seems broken. The orientation changes are handled correctly when using setColumnCountPortrait or setColumnCountLandscape, but using setColumnCount leads to #70, #105, #134, #138.

imminent commented 10 years ago

why can't you define an integer resource gridColumnCount and bucket it in the corresponding width buckets (like sw600dp, sw720dp)? Gives you the same power, through using the Android resource management.

stanch commented 10 years ago

@imminent I’m creating the grid programmatically, and setColumnCount method seems broken. I’m not sure your method works in this case. Of course it is possible to achieve the desired effect even with setColumnCountPortrait and setColumnCountLandscape by checking the current orientation and screen dimensions. But this all is orthogonal to the point: determining the column count from orientation makes no sense, because one device in portrait is wider than another device in landspace.

imminent commented 10 years ago

You could set them both to the same dimen value, which uses the resource qualifiers to change based on screen size, as described earlier. That means the value won't change based on orientation, but rather based on screen size as you want. On Oct 1, 2014 8:28 AM, "Nick" notifications@github.com wrote:

@imminent https://github.com/imminent I’m creating the grid programmatically, and setColumnCount method seems broken. I’m not sure your method works in this case. Of course it is possible to achieve the desired effect even with setColumnCountPortrait and setColumnCountLandscape by checking the current orientation and screen dimensions. But this all is orthogonal to the point: determining the column count from orientation makes no sense, because one device in portrait is wider than another device in landspace.

— Reply to this email directly or view it on GitHub https://github.com/etsy/AndroidStaggeredGrid/issues/148#issuecomment-57482758 .

dzlab commented 9 years ago

@imminent can you describe further your solution (with some code for instance)

lawloretienne commented 9 years ago

@imminent, is this issue related to https://github.com/etsy/AndroidStaggeredGrid/issues/156?

stanch commented 9 years ago

@lawloretienne I’m not @imminent, but as the OP I would say yes :)

imminent commented 9 years ago

@dzlab

res/values/integers.xml -> <integer name="grid_column_count">2</integer>
res/values-sw600dp-v14/integers.xml -> <integer name="grid_column_count">3</integer>

then set the Grid View like so:

<com.etsy.android.grid.StaggeredGridView
  android:layout_width="match_parent"
  android:layout_height="wrap_content"
  app:column_count_landscape="@integer/grid_column_count"
  app:column_count_portrait="@integer/grid_column_count" />

That way the column count only changes when the Integer resource changes (which in this case is on screen size)

dzlab commented 9 years ago

thanks @imminent i've end up to a similar solution that combines: -the use of folders like values-small, values-medium, values-xlarge for the column number, with -code that chooses the size of the picture (i've different sizes) to display based on the screen density

maudem-m commented 9 years ago

Indeed, I just add this in my Fragment and it has been working like a charm! ^^

@Override
    public void onConfigurationChanged(Configuration newConfig) {
        super.onConfigurationChanged(newConfig);
        if(newConfig.orientation==Configuration.ORIENTATION_PORTRAIT) {
            mGridView.setColumnCountPortrait(getResources().getInteger(R.integer.grid_posts_column_count));
        }else if(newConfig.orientation==Configuration.ORIENTATION_LANDSCAPE){
            mGridView.setColumnCountLandscape(getResources().getInteger(R.integer.grid_posts_column_count));
        }
}