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

Day view #247

Merged
merged 14 commits into from
Jun 3, 2020
Merged

Day view #247

merged 14 commits into from
Jun 3, 2020

Conversation

araujoarthur0
Copy link
Collaborator

Related issue

#96 Minimalist View

Context / Background

  • Briefly explain the purpose of the PR by providing a summary of the context

This PR is adding a Day View to the calendar, which focuses the view to just one day, making it a "minimalist" version of sorts.

What change is being introduced by this PR?

  • How did you approach this problem?
  • What changes did you make to achieve the goal?
  • What are the indirect and direct consequences of the change?

There is a new DayCalendar class based on the original Calendar class, for this to work. I'm using the same infrastructure and adding a Factory class to be able to instantiate correctly the different Calendar classes.
The view of the new calendar is no longer based on tables: I chose flexbox instead of tables to be able to change the size of the elements in a way that is not tied to rows and columns in a table.
The "view" type is now a new preference: month and day options.
One can switch through the preferences menu, or through the new switch button on the top left of the calendar.

How will this be tested?

  • How will you verify whether your changes worked as expected once merged?

Ran tests, and expect daily testing on the dev channel.

day

@codecov
Copy link

codecov bot commented May 30, 2020

Codecov Report

Merging #247 into master will increase coverage by 9.19%.
The diff coverage is 67.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
+ Coverage   46.98%   56.18%   +9.19%     
==========================================
  Files          12       12              
  Lines         996     1196     +200     
  Branches      179      214      +35     
==========================================
+ Hits          468      672     +204     
+ Misses        461      460       -1     
+ Partials       67       64       -3     
Impacted Files Coverage Δ
js/calendar.js 0.00% <0.00%> (ø)
main.js 0.00% <0.00%> (ø)
src/preferences.js 0.00% <0.00%> (ø)
src/workday-waiver.js 0.00% <0.00%> (ø)
js/user-preferences.js 83.83% <52.94%> (-4.12%) ⬇️
js/classes/Calendar.js 80.61% <92.85%> (+13.85%) ⬆️
js/themes.js 100.00% <100.00%> (ø)

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 512beff...54800d5. Read the comment docs.

@araujoarthur0
Copy link
Collaborator Author

Tried breaking it down the most, but the last one is just a big commit, or it wouldn't make sense. Sorry guys heuaheuha

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

This is looking great already. A couple notes though:

  • We need to know where the icon came from to give proper credit, unless you made it :D. I think I'd go with something like this: https://www.flaticon.com/free-icon/minimize_2749251?term=minimalism&page=1&position=61 or similar.
  • The preferences window is, for some reason, not wide enough now, and one of the days is dropping to a second line.
  • Also on the preferences window, it became long enough to have a scrollbar. I think we aim to have it show every option on pop-up, unless it becomes too large.
  • I don't know what to do with the "hide working-days" option when it's in the day view.
  • We should add some testing to the new preference

@araujoarthur0
Copy link
Collaborator Author

Switched the icon with the one you sent :)
About preferences, I increased the window height and both points got addressed.
About hide working days, my intention by adding the "(month view)" commentary there was to make it explicit that it only works with day view. I don't think this view benefits from that preference, there would be too many things to limit, and navigation might be hampered.

Finally, I'll add some tests later.

@araujoarthur0
Copy link
Collaborator Author

Tests added, similar to the ones for the month calendar, and about the instance change.

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

This looks great. I'm a bit divided over the tests though, because it seems like it duplicates a lot of other tests, but I don't know what to think about them right now.

__tests__/__renderer__/classes/Calendar.js Outdated Show resolved Hide resolved
__tests__/__renderer__/classes/Calendar.js Show resolved Hide resolved
assets/switch.svg Show resolved Hide resolved
@tupaschoal
Copy link
Collaborator

Ah! You'd also need to please add a line (sorted) about this new feature on changelog.txt

@araujoarthur0
Copy link
Collaborator Author

I have to take a look since it looks like the punch button is getting more than one click event after messing around with the calendar.

@tupaschoal
Copy link
Collaborator

Maybe when you switch days it's registering the punch again?

@araujoarthur0
Copy link
Collaborator Author

Fixed the punch issue by throwing the logic outside.
I'll now add a way for the app to load by default on the smaller size when it's set to day view. Right now it's loading big and then decreasing the size.

@araujoarthur0
Copy link
Collaborator Author

And done :) Now it starts small if set to Day view and not maximized.

Copy link
Owner

@thamara thamara left a comment

Choose a reason for hiding this comment

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

Looks awesome!

@thamara
Copy link
Owner

thamara commented Jun 3, 2020

@tupaschoal, do you have any other consideration on this?

@tupaschoal
Copy link
Collaborator

No, merge it!

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

Looks like it needs to be rebased @araujoarthur0

@araujoarthur0
Copy link
Collaborator Author

Rebased :)

@thamara thamara merged commit 7a643d6 into thamara:master Jun 3, 2020
@araujoarthur0 araujoarthur0 deleted the day-view branch June 19, 2020 01:38
@tupaschoal tupaschoal added this to the v1.5.3 milestone Jun 23, 2020
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.

None yet

3 participants