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: Issue 189, implement agendaEvent slot #220

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

robairhome
Copy link

@robairhome robairhome commented Dec 9, 2023

Checklist

Please put "X" in the below checkboxes that apply::

  • If committing a bugfix, I have tested it in different browsers (Chrome, Firefox, Safari).
  • [X ] If committing a new feature, I have first submitted an issue (Please note: you are free to open PRs for non-issued features, but opening an issue increases your chance of a successful PR).
  • If committing a new feature, I have also written an appropriate test suite for it.

I have tested the following:

  • [ X] Qalendar component in month mode.
  • [ X] Qalendar component in week mode.
  • [ X] Qalendar component in day mode.
  • [ X] All of the above modes on emulated mobile view.
  • [ X] Dragging and dropping events.
  • [ X] Resizing events in day/week modes.
  • [ X] Clicking events to open event dialog.

This PR solves the following problem**.

There is currently no custom slot for agendaEvent. Have implemented it in line with Tom's suggestion. However, I need a little assistance. I have to change the modeType type to allow a modeType to be agenda, but while it works fine in dev it throws an error when I try to build (related to the modification of modeType). Alternatively, I can fix it by removing agenda from modeType, but then it means the custom agenda will only show when isCustom = true, vs the other modes which allow it to be turned on with isCustom = true or isCustom = ['week']

How to test this PR**.

Custom agenda slot is added to dev. Set isCustom to true or 'agenda' and run to see it.

@tomosterlund
Copy link
Owner

From a quick look, it looks correct to me that you have added it to the modeType. Makes me wonder what we were thinking back when we didn't do it that way 😄

I can have a look at the TS-errors

@tomosterlund tomosterlund self-requested a review December 9, 2023 09:07
@robairhome
Copy link
Author

Thanks, I'll look at the changes you've suggested tonight and get them resolved!

@tomosterlund
Copy link
Owner

Cool! I pushed a little something to fix the TS-error in Header.vue

@coveralls
Copy link

coveralls commented Dec 9, 2023

Coverage Status

coverage: 97.49% (-0.1%) from 97.603%
when pulling 98ac613 on robairhome:master
into 63653bb on tomosterlund:master.

robairhome and others added 3 commits December 10, 2023 18:36
Per issue 189, this implements custom agenda events via an agendaEvent slot. This will be displayed on mobile, similar to the monthEvent and dayEvent

Update types.ts

Update Header.vue

Modify css so that custom event doesn't inherit colors

Per discussion with Tom, modifying the css so that the custom agenda event doens't modify the css.

fix cypress

Update cypress.config.ts
@robairhome
Copy link
Author

robairhome commented Dec 10, 2023

Hey,

I've made those changes. The last thing I need to do is the unit tests.

I was looking through the code but it didn't jump out at me how you determine if your in mobile or desktop view to be in "agenda" mode. I need to mock this in the unit tests to create the customAgenda test. Any chance you know this off the top of your head (or approximately where I should snoop around)?

Cheers

@tomosterlund
Copy link
Owner

Hi. I've been trying to write some unit tests for the slots before, and was never happy with the way they ended up: so I deleted them and now rely on the Cypress tests instead. For me it's fine the way you've tested it now. The most critical piece of logic added here:

isCustomAgendaEvent() {
      // Logic to determine if this event should use a custom agenda event slot
      if (Array.isArray(this.calendarEvent.isCustom)) {
        return this.calendarEvent.isCustom.includes('agenda');
      }
      return this.calendarEvent.isCustom || false;
    },

is implicitly put under test through a combination of the Cypress-test you wrote, and 09-small-qalendar.cy.ts

@tomosterlund
Copy link
Owner

tomosterlund commented Dec 11, 2023

I now realized why we didn't add the agenda to the modeType originally: the function that decides whether to render an agenda or the larger month, depends on the screen size, so from the perspective of the implementer, an 'agenda' is not its own mode, but rather a 'month'. Of course, for the isCustom property there will need to be an exception here. This property does now know an agenda mode.

I just broke something, and will need to look through why.

One last thing you could do, is perhaps to document the usage of 'agenda' in the isCustom property.

@robairhome
Copy link
Author

robairhome commented Dec 11, 2023

Great, I'll do the docs update edit: sorry tomorrow night - today was busier than expected.

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