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

fix(event-click): clicking actual events now triggers eventClicked #568

Closed
wants to merge 2 commits into from

Conversation

dwknippers
Copy link
Contributor

Event titles used to only trigger eventClicked events. This doesn't make sense, so trigger eventClicked when the actual event item (week or day) is clicked.

@codecov-io
Copy link

codecov-io commented May 30, 2018

Codecov Report

Merging #568 into master will decrease coverage by 0.12%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #568      +/-   ##
==========================================
- Coverage   96.66%   96.54%   -0.13%     
==========================================
  Files          34       34              
  Lines         600      608       +8     
  Branches       62       66       +4     
==========================================
+ Hits          580      587       +7     
  Misses          1        1              
- Partials       19       20       +1
Impacted Files Coverage Δ
...rc/modules/day/calendar-all-day-event.component.ts 100% <ø> (ø) ⬆️
...c/modules/day/calendar-day-view-event.component.ts 100% <100%> (ø) ⬆️
...modules/week/calendar-week-view-event.component.ts 92.3% <75%> (-7.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b20f821...15a73cc. Read the comment docs.

@mattlewis92
Copy link
Owner

This is actually by design, really the event should be called eventTitleClicked - if you have it on the parent element then it will trigger every time you click an event action as well. I'm open to suggestions here on how we could handle this scenario gracefully but I can't think of anything off the top of my head.

@etwillbefine
Copy link
Collaborator

Can't we just stop the event propagation when clicked on one of the actions by using stopPropagation on the js-event?

@mattlewis92
Copy link
Owner

Unfortunately that doesn't work when people use hammerjs and then you have to litter the codebase with hacks like this: https://github.com/mattlewis92/angular-calendar/blob/master/src/modules/month/calendar-month-view.component.ts#L344-L349

@etwillbefine
Copy link
Collaborator

Hmm if its already there, we could put in a directive if its about the hack itself not about performance or whatever. WDYT? I think its worth it. As smaller the title and bigger the screen there is a lot of space not clickable right now.

@dwknippers
Copy link
Contributor Author

Ah event actions totally slipped my mind and testing. I'll take a look if I it's possible to handle the scenario gracefully.

@dwknippers
Copy link
Contributor Author

dwknippers commented May 31, 2018

Latest change simply ignores event propagation from inner elements as kind of a hack.
This isn't ideal, but it does seem to work. Please note that it would be possible to stop propagation if 'domEvents' was set to true in the project Hammerjs would be used (https://hammerjs.github.io/tips/).

So far I haven't been able to test using Hammerjs as I'm unfamiliar with the framework, let me know if there are any issues with it.

Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

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

So sorry about the late reply on looking into this! I've just checked it out locally and it looks like this change breaks the cursor changing when you mouse over the edges of a resizable event, could you take a look please? Thanks 😄 Other than that, LGTM

@dwknippers
Copy link
Contributor Author

Oh that is odd, I will look into it! Thanks for the review and dealing with me, still new to this :)

mattlewis92 pushed a commit that referenced this pull request Jun 24, 2018
BREAKING CHANGE: eventClicked now fires whenever the event container instead of the event title is clicked

Closes #568
mattlewis92 pushed a commit that referenced this pull request Jun 24, 2018
BREAKING CHANGE: eventClicked now fires whenever the event container instead of the event title is clicked

Closes #568
@mattlewis92
Copy link
Owner

Merging this cleanly got a bit funky, so I've done it locally and fixed the issues with the cursor here: 403e127 (I also found a way to make the stopPropagation work seamlessly without any changes to the rest of the code 🎉 )

It'll be part of 0.26, if you want to use it today then instructions for trying out that branch are over in the PR: #589

Thanks again for your contribution! 👍

@dwknippers
Copy link
Contributor Author

Thank you, I meant to look into this but didn't find any time :-)

@mattlewis92 mattlewis92 mentioned this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants