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

Tap on Backgrounds for DateEvents and AllDayEvents #20

Merged
merged 14 commits into from
Jun 25, 2020
Merged

Tap on Backgrounds for DateEvents and AllDayEvents #20

merged 14 commits into from
Jun 25, 2020

Conversation

raLaaaa
Copy link
Contributor

@raLaaaa raLaaaa commented Jun 21, 2020

Closes: #18

I thought I'd issue a pull request at the current state regarding #18.

Background taps are currently recognized across DateEvents and AllDayEvents. Tapping on headers is currently not implemented yet.
The pull request adapted the example with a snackbar function.

There are some things on my mind and I'm looking forward to your feedback:

  1. Is the name onCreateEvent and onCreateAllDayEvent really appliceable? Maybe we should change it to somehting like onBackgroundTap or onCellTap?

  2. Regarding DateEvents I'm not quite sure why when calculating the position of the tapped cell I need to add a +1 here:

final dateAndTime = DateTime(date.year, date.monthOfYear, date.dayOfYear, tappedCell.toInt() + 1);
  1. For allday events I even had to do +2 for the day.
final dateAndTime = DateTime(date.year, date.monthOfYear, date.dayOfYear + 2);

I'm not really sure about the calculations at all. The offset issue with the days might be related to wrong rounding? Might also be a conversion problem related to final startTime = LocalDateTime.dateTime(dateAndTime) I think? Maybe you could shed some light into this and the calculation at all. Also the time of the giving back object is always 23:00:00 eventhough it should be 00:00:00

  1. I think there are still some parts where I could make the code partly more elegant. Advices are appreciated :) !

Copy link
Owner

@JonasWanke JonasWanke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply, and thanks already for your work! I've added some comments (sorry for the amount; some of it is just about formatting).

  1. I like onBackgroundTap, which makes clear that you can also use that listener for other purposes. We can probably merge both parameters of the Timetable widget into a single one, which was the idea behind the additional boolean parameter (true if the tap was in the header area, false if the tap occurred in the part-day area).
  2. I think I found the reason for the + 1: It might be from the conversion between LocalDateTime from time_machine and Dart's DateTime: DateTime by default uses the local timezone, whereas LocalDateTime has no knowledge about timezones at all. See my inline comment for an idea of how to fix it.
  3. It sounds like a conversion problem as well, which would also explain the 23:00:00. See my inline comment for more details.
  4. That would be the comments about extracting lambdas into private helper methods, but otherwise, your code looks good!

lib/src/content/multi_date_content.dart Outdated Show resolved Hide resolved
lib/src/content/multi_date_content.dart Outdated Show resolved Hide resolved
lib/src/content/multi_date_content.dart Outdated Show resolved Hide resolved
lib/src/content/multi_date_content.dart Outdated Show resolved Hide resolved
lib/src/content/timetable_content.dart Outdated Show resolved Hide resolved
lib/src/header/all_day_events.dart Outdated Show resolved Hide resolved
lib/src/header/all_day_events.dart Outdated Show resolved Hide resolved
lib/src/header/all_day_events.dart Outdated Show resolved Hide resolved
lib/src/header/all_day_events.dart Outdated Show resolved Hide resolved
lib/src/header/all_day_events.dart Outdated Show resolved Hide resolved
@JonasWanke JonasWanke added C: Example Component: Example app C: Timetable Component: The actual timetable package T: Feature Type: :tada: New Features labels Jun 22, 2020
@raLaaaa
Copy link
Contributor Author

raLaaaa commented Jun 22, 2020

Thanks for your detailed feedback Jonas.
I tried to implement all your hints.

  1. The +1 and +2 offset indeed got caught by time_machine and flutters timeformat. Using time_machine directly fixes it.

  2. I increased tap precision. I did not use TimeConstants.millisecondsPerDay though. See

3a59221

From my feeling I'd say there is a more elegant way. I'm not quite sure about this but it works for now.

  1. Is this implementation of attaching the listener only when it's not null legit?
  onTapUp: widget.onEventBackgroundTap != null
                    ? (details) {
                       ...
                      }
                    : null,

We can probably merge both parameters of the Timetable widget into a single one, which was the idea behind the additional boolean parameter (true if the tap was in the header area, false if the tap occurred in the part-day area).

This still needs to be done. Otherwise I think we are nearly done? :)

I'd seperate the tapping inside the header function to a seperate issue and not include in this pull request. Even though it's related I think I'd create a different issue for semantical reasons.

Copy link
Owner

@JonasWanke JonasWanke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Is there a reason you didn't use millisecond precision? That would make the code much cleaner:
final millis = details.localPosition.dy / constraints.maxHeight
    * TimeConstants.millisecondsPerDay;
final time = LocalTime.sinceMidnight(Time(milliseconds: millis.floor()));
// Note: I haven't tested this code

Also, users of this package can decide the necessary precision by themselves.

  1. Yes, that's legit. That way, we can also drop the if (widget.onEventBackgroundTap != null) guard (and that in all_day_events.dart) as the listener cannot be null when it should be invoked.
    By extracting the private helper function, I meant to extract the whole lambda, though. That way, there isn't an additional ≈ 10 lines of logic inside the already nested build method whose primary purpose is to create the UI. You can simply move the whole lambda content to the helper methods you already created.

  2. I think we might be talking about different "header" functions. My idea was that the callbacks currently called onAllDayEventBackgroundTap and onEventBackgroundTap can be merged into a single callback onEventBackgroundTap. If isAllDay is set to false when that callback is invoked, the user tapped somewhere in the part-day content area. If isAllDay is true, the user tapped in the all-day event area of the header. Sorry for the bad wording :/
    Also, I just noticed that the OnCreateEventCallback-typedef can now be renamed to something like OnEventBackgroundTapCallback.

But apart from these (now relatively minor) comments, we're done :)

@@ -55,7 +55,7 @@ class TimetableContent<E extends Event> extends StatelessWidget {
child: MultiDateContent<E>(
controller: controller,
eventBuilder: eventBuilder,
onCreateEvent: onCreateEvent
onEventBackgroundTap: onEventBackgroundTap
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
onEventBackgroundTap: onEventBackgroundTap
onEventBackgroundTap: onEventBackgroundTap,

lib/src/header/all_day_events.dart Outdated Show resolved Hide resolved
@raLaaaa
Copy link
Contributor Author

raLaaaa commented Jun 24, 2020

Great feedback. The code is much cleaner now.

  • I did not use the millisecond method because I could somehow not wrap my hand around even though it's super simple and easy.

  • Extracting the entire lambda into a helper function definetly makes more sense and makes to code much cleaner.

  • I merged the two listeners into one now. Thanks for clarifying! What I was thinking about when talking about listener for header was another seperated listener which allows us do receive taps from the headline. See my image:

image

EG. when tapping where the red arrow points at.

Merging two listeners into one eases the usement of the librabry and makes the code much cleaner. I'm still learning so thanks for your awesome feedback so far! Really appreciated.

@JonasWanke
Copy link
Owner

Now that everything works as intended and the code looks fine, unless you have anything more you want to add, we can merge this PR!

@JonasWanke JonasWanke marked this pull request as ready for review June 25, 2020 06:08
@raLaaaa
Copy link
Contributor Author

raLaaaa commented Jun 25, 2020

Now that everything works as intended and the code looks fine, unless you have anything more you want to add, we can merge this PR!

I think we are good to go :) Cheers!

@JonasWanke JonasWanke merged commit cdc7a86 into JonasWanke:master Jun 25, 2020
@JonasWanke JonasWanke changed the title WIP: Tap on Backgrounds for DateEvents and AllDayEvents (draft) Tap on Backgrounds for DateEvents and AllDayEvents Jun 25, 2020
@JonasWanke
Copy link
Owner

This is now published as part of v0.2.4. Thanks for contributing!

@raLaaaa
Copy link
Contributor Author

raLaaaa commented Jun 26, 2020

Thanks for the opportunity @JonasWanke . Hopefully I can contribute some more in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Example Component: Example app C: Timetable Component: The actual timetable package T: Feature Type: :tada: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Listener when tapping the background (e.g. for creating an event)
2 participants