Add dynamic height options #24

Bwaim · 2019-01-09T12:58:01Z

This PR contains :

A simplification of the header height management
A new option to have dynamic hour height based on the available height
A change of the hour precision to make the new option works better
Add of a new example to demonstrate the new option in a ConstraintLayout

I could adapt it if you accept previous PR as it has cross impact

thellmund · 2019-01-09T17:39:17Z

Could you elaborate on the effects that your changes have? All I can observe is the increased height of all-day event chips.

Bwaim · 2019-01-10T09:49:59Z

The first commit (e3a9765) has no visual impact. It's just an harmonization of the use of headerHeight to facilitate the positionning. It has also some minor corrections like the size of the all day event chip which was not using the existing parameter allDayEventHeight.

The third commit(ac772f0) just increase the precision of the postionning of hour separator has it was needed for the option.

The new option dynamicHourHeight allow to fit all hours in a specified height if you want to have all your calendar in the same page. It allows to not have to define a specific height for the component. I added an example to play with it dynamically (Basic Example (inside constraint layout)) :

Bwaim · 2019-01-10T15:41:48Z

i added a fix for the position of the day label (5a88afe) and a new parameter headerMarginBottom.
The header is composed like this :

thellmund · 2019-01-16T17:48:10Z

There is an issue when scrolling horizontally between days, where all-day event chips don’t disappear correctly.

thellmund · 2019-01-16T17:49:51Z

library/src/main/java/com/alamkanak/weekview/WeekViewConfig.java

@@ -39,6 +39,7 @@
    int headerRowBackgroundColor = Color.WHITE;
    int headerRowPadding = 10;
    int todayHeaderTextColor = Color.rgb(39, 137, 228);
+    int headerMarginBottom = 0;


Currently, headerRowPadding is used to determine the padding below the all-day event chips. By introducing this new attribute, we make yet another breaking change for users of this library.

I agree that it's a breaking change. The thing is that actually, the header is not well computed, i mean the user can't have a control to change the 3 spaces available correctly.
For example, the headerMarginBottomactually couldn't be controlled, i just added the possibility to the user to change it.
How would you want to compute the header spaces ? i'll think about how to change it without impact or less impact.

Moreover, the day label was not well placed and there will be impact on existing project. So what do you think is the best ?
Edit : The vertical position of the day label is computed as this :
val y = drawingConfig.headerTextHeight + config.headerRowPadding

There’s a lot happening in this pull request already, so I think it’d be best to leave it as it is for now. Feel free to pick up the topic in a separate pull request, if you like.

library/src/main/res/values/attrs.xml

sample/src/main/AndroidManifest.xml

sample/src/main/res/values/strings.xml

sample/src/main/res/layout/activity_constraint.xml

thellmund · 2019-01-16T18:04:53Z

library/src/main/java/com/alamkanak/weekview/WeekViewDrawingConfig.java

+          headerHeight += config.headerRowBottomLineWidth;
+      }
+      if (hasEventInHeader) {
+          headerHeight += config.allDayEventHeight;


Thinking back to my refactoring, allDayEventHeight is something that I wanted to deprecate, but forgot to do. The reason is that while the event chip might become taller, the event text size stays the same, resulting in a waste of space (just take a look at the screenshot you posted above).

There are a couple options for how to continue:

Continue to calculate the chip height based on the text size and officially deprecate allDayEventHeight.

Adjust the text size of all-day events to fit the height of the chip (allDayEventHeight).

Introduce an allDayEventTextSize, which is applied to all-day event chips. If not specified, it would fall back to the eventTextSize, so it would not be a breaking change.

What’s your take on this, @Bwaim?

I think your propositions 1 and 3 are good. As you said, we should try to reduce the header size to the minimum.
Added to the deprecation of allDayEventHeight, maybe we could add a new parameter maxAllDayEventHeight to prevent the chip to be too big. What do you think about it ?

I think the proposition 2 is not good because if the event label is very long, we will have a problem to decide the text size to use.

Let’s try idea 3. It gives the most control and wouldn’t change anything for users of this library.

In last commit, i implemented your propositions 1 and 3, do you prefer i just let proposition 3 and revert the deprecation of allDayEventHeight ? Should i remove the maxAllDayEventHeight parameter i added ?

I don’t think maxAllDayEventHeight is necessary – the height of the chip will be determined by allDayEventTextSize. You can remove the former.

The deprecation of allDayEventHeight is fine.

Bwaim · 2019-01-17T16:51:03Z

I commited a new version taking into account the remarks.
Just 2 remarks are not changed yet :

The use of headerRowPadding
The use of allDayEventHeight

- Write the day label correctly, using the baseline - Use the allDayEventHeight parameter for the size of all day event chip - Correct computation of the day label height

- Fix the bug of the all day event chip displayed in the upper left corner - Rename the parameter dynamicHourHeight to showCompleteDay - Rename the example to "Basic Example (showing complete day)" - Correct the indentation of activity_constraint.xml

Bwaim · 2019-01-18T14:11:57Z

I commited a new version taking into account the remark concerning the use of allDayEventHeight.
I wait your opinion about how you want to compute the header height.

- Remove the parameter allDayEventHeight - Add a parameter allDayEventTextSize - Add a parameter maxAllDayEventHeight - Calculate the chip height based on the text size and maxAllDayEventHeight

thellmund · 2019-02-01T08:16:27Z

@Bwaim Sorry for the delay. I’m having exams soon, so I’m not sure when I’ll find the time. But I won’t forget about your two pull requests 😉

Bwaim · 2019-02-01T08:20:53Z

@thellmund No problem, exams first, good luck !

thellmund · 2019-02-13T15:21:40Z

@Bwaim Looked at your latest changes as a bit of studying procrastination 😉I noticed two small issues:

The text weight of the all-day events is off. It should be bold, but is normal.
There’s no longer the padding below all-day event chips. Please add that back.

Besides that, the rest seems fine. Just these two things, and then we can merge this thing 👌

thellmund · 2019-02-13T17:19:57Z

@Bwaim Nevermind, I will merge this commit and resolve the issues 😊

Bwaim · 2019-02-14T07:57:12Z

2. There’s no longer the padding below all-day event chips. Please add that back.

@thellmund As explained in my comment of the 10 January, the padding below all-day event chips is now controlled with the parameter headerRowPadding

Bwaim mentioned this pull request Jan 10, 2019

Add some time column options #16

Merged

Bwaim force-pushed the add_dynamic_height_options branch from 335ee95 to a979def Compare January 10, 2019 14:18

thellmund requested changes Jan 16, 2019

View reviewed changes

Bwaim force-pushed the add_dynamic_height_options branch 3 times, most recently from 3a67798 to 52cad28 Compare January 17, 2019 16:48

Bwaim force-pushed the add_dynamic_height_options branch from 52cad28 to c63d1cd Compare January 18, 2019 10:00

Bwaim added 7 commits January 18, 2019 11:04

Fix: Simplify headerHeight management

2524aa4

Feature: add an option to have dynamic hour height

c72892e

Feature: change the hour height precision from int to float

5c4bcb8

Add an example with dynamic hour height inside a constraint layout

f2a0fdf

Fix: correct size and position in header

54b9438

- Write the day label correctly, using the baseline - Use the allDayEventHeight parameter for the size of all day event chip - Correct computation of the day label height

Feature: add a new parameter headerMarginBottom

18e41a1

Fix: changes according to the PR remarks

f04306c

- Remove the parameter allDayEventHeight - Add a parameter allDayEventTextSize - Add a parameter maxAllDayEventHeight - Calculate the chip height based on the text size and maxAllDayEventHeight

thellmund approved these changes Feb 13, 2019

View reviewed changes

thellmund merged commit 807303e into thellmund:develop Feb 13, 2019

cs8898 mentioned this pull request Feb 13, 2019

ConstraintActivity ArrayIndexOutOfBoundsException #34

Closed

Bwaim deleted the add_dynamic_height_options branch February 14, 2019 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic height options #24

Add dynamic height options #24

Bwaim commented Jan 9, 2019

thellmund commented Jan 9, 2019

Bwaim commented Jan 10, 2019

Bwaim commented Jan 10, 2019

thellmund commented Jan 16, 2019

thellmund Jan 16, 2019

Bwaim Jan 17, 2019 •

edited

Loading

Bwaim Jan 18, 2019 •

edited

Loading

thellmund Feb 13, 2019

thellmund Jan 16, 2019

Bwaim Jan 17, 2019

thellmund Jan 24, 2019

Bwaim Jan 24, 2019

thellmund Feb 13, 2019

Bwaim commented Jan 17, 2019

Bwaim commented Jan 18, 2019

thellmund commented Feb 1, 2019

Bwaim commented Feb 1, 2019

thellmund commented Feb 13, 2019

thellmund commented Feb 13, 2019

Bwaim commented Feb 14, 2019

Add dynamic height options #24

Add dynamic height options #24

Conversation

Bwaim commented Jan 9, 2019

thellmund commented Jan 9, 2019

Bwaim commented Jan 10, 2019

Bwaim commented Jan 10, 2019

thellmund commented Jan 16, 2019

thellmund Jan 16, 2019

Choose a reason for hiding this comment

Bwaim Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Bwaim Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

thellmund Feb 13, 2019

Choose a reason for hiding this comment

thellmund Jan 16, 2019

Choose a reason for hiding this comment

Bwaim Jan 17, 2019

Choose a reason for hiding this comment

thellmund Jan 24, 2019

Choose a reason for hiding this comment

Bwaim Jan 24, 2019

Choose a reason for hiding this comment

thellmund Feb 13, 2019

Choose a reason for hiding this comment

Bwaim commented Jan 17, 2019

Bwaim commented Jan 18, 2019

thellmund commented Feb 1, 2019

Bwaim commented Feb 1, 2019

thellmund commented Feb 13, 2019

thellmund commented Feb 13, 2019

Bwaim commented Feb 14, 2019

Bwaim Jan 17, 2019 •

edited

Loading

Bwaim Jan 18, 2019 •

edited

Loading