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

feat(weekView): Add height and scrollbar #229

Closed
wants to merge 18 commits into from

Conversation

ElieSauveterre
Copy link
Contributor

Add parameter "maxHeight" to have a fixed calendar height with a vertical scrollbar.
Useful when there is a lot of events for the same day.

@codecov-io
Copy link

codecov-io commented Jun 2, 2017

Codecov Report

Merging #229 into master will decrease coverage by 0.99%.
The diff coverage is 16.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #229   +/-   ##
=======================================
- Coverage   96.43%   95.44%   -1%     
=======================================
  Files          29       29           
  Lines         477      483    +6     
  Branches       49       50    +1     
=======================================
+ Hits          460      461    +1     
- Misses          0        4    +4     
- Partials       17       18    +1
Impacted Files Coverage Δ
src/components/week/calendarWeekView.component.ts 89.24% <16.66%> (-5.01%) ⬇️

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 6d51183...4ec860c. Read the comment docs.

@@ -49,6 +49,7 @@ import { CalendarUtils } from '../../providers/calendarUtils.provider';
(dayClicked)="dayClicked.emit($event)"
(eventDropped)="eventTimesChanged.emit($event)">
</mwl-calendar-week-view-header>
<div class="cal-events" [style.height]="maxHeight+'px'" [style.overflowY]="maxHeight ? 'auto' : ''">
<div *ngFor="let eventRow of eventRows" #eventRowContainer class="cal-events-row">
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than adding another option I'd prefer just to add the wrapping div and let users add the styles themselves through CSS.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you also fix the indentation of the child element


// With a scrollbar during the drag, the event is only visible inside the calendar.
// The "fixed" position bring the event on top, even when dragged outside the calendar.
if (this.allowDragOutside && this.maxHeight) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's safe to always do if (this.allowDragOutside) {

// With a scrollbar during the drag, the event is only visible inside the calendar.
// The "fixed" position bring the event on top, even when dragged outside the calendar.
if (this.allowDragOutside && this.maxHeight) {
event.style.left = ((event.getBoundingClientRect().left / document.body.clientWidth ) * 100) + '%';
Copy link
Owner

Choose a reason for hiding this comment

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

getBoundingClientRect() is quite expensive, can you compute it just once on the line above. Rather than using a percentage can it not just be event.getBoundingClientRect().left + 'px' ? What happens if the user cancels and the event isn't dropped out of the calendar?

Maybe it's better to put this functionality into the drag and drop module as this would be helpful to anyone using drag and drop within a scrollable box. What do you think?

@ElieSauveterre
Copy link
Contributor Author

@mattlewis92 I have removed the extra parameter and improve the positioning of the event.

Yes, I agree that this features can be useful for other components.

mattlewis92 pushed a commit that referenced this pull request Jun 25, 2017
@mattlewis92
Copy link
Owner

Hey! 👋 Sorry about the extremely late reply on this one, I've been away on holiday the past few weeks / not had much time for open source. On reflection I realised that this and the other PR (#232) you sent can actually be implemented yourself in userland by extending the built in component and adding all the functionality you need to your app. I've put together a plunker that demonstrates how to do this here: http://plnkr.co/edit/5KyUBC0lnfMsYMcVFAR9?p=preview

In the interests of keeping the core library as minimal as possible, I'm also going to remove the allowDragOutside flag in the next release. I've included the functionality in that plunker so you should just be able to copy it into your app and be good to go. If you need any more information feel free to give me a shout 😄

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.

3 participants