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

DevTools shortcut on Preferences and Waiver windows #254

Merged
merged 4 commits into from
Jun 4, 2020

Conversation

araujoarthur0
Copy link
Collaborator

Related issue

#245

Context / Background

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

Adding DevTools shortcut on Preferences and Waiver windows.

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's a new listener waiting for CTRL+SHIFT+I to be pressed in the preferences and waiver windows, to toggle chrome dev tools on those windows.

How will this be tested?

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

All working.

@codecov
Copy link

codecov bot commented May 31, 2020

Codecov Report

Merging #254 into master will decrease coverage by 1.10%.
The diff coverage is 70.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   56.18%   55.08%   -1.11%     
==========================================
  Files          12       13       +1     
  Lines        1196     1220      +24     
  Branches      214      218       +4     
==========================================
  Hits          672      672              
- Misses        460      480      +20     
- Partials       64       68       +4     
Impacted Files Coverage Δ
js/calendar.js 0.00% <0.00%> (ø)
js/window-aux.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%> (ø)
js/classes/Calendar.js 80.61% <92.85%> (ø)
... and 3 more

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 0de90f2...f7ee3e7. Read the comment docs.

src/preferences.js Outdated Show resolved Hide resolved
@araujoarthur0
Copy link
Collaborator Author

Added a new window-aux.js file with the devtools binding function.
Took the chance to also extract alert and confirm functions to this aux file, and fixed the error string for when waiver periods don't contain working days.

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.

Very nice!

@thamara thamara added this to the v1.5.2 milestone Jun 3, 2020
js/window-aux.js Show resolved Hide resolved
@@ -14,6 +14,7 @@
- Fix: [#250] Silently ignoring waiver on non-working day or non-working day range
- Enhancement: [#228] Improved performance of TTL - Now moving through the calendar is much faster
- Enhancement: [#152] Adding a "Copy" option in the "About message", making it easier to copy information when opening an issue
- Enhancement: [#245] DevTools shortcut on Preferences and Waiver windows
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you squash this changelog commit with the first one? I think it also would be useful to say something like:

Suggested change
- Enhancement: [#245] DevTools shortcut on Preferences and Waiver windows
- Enhancement: [#245] DevTools shortcut (Ctrl+Shift+I) on Preferences and Waiver windows

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean not having a separate commit for the changelog?
When accepting the merge request are you not able to squash them on demand? (real question, I've never accepted one hahaha)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I mean, you're basically changing the changelog (duh ahah) because of the first commit, so it should go together with it.

We can either squash them all together or have them all separate via the GitHub interface. We can squash portions using the CLI

Copy link
Owner

Choose a reason for hiding this comment

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

I've updated the changelog directly in master after merging the PR by mistake. (sorry again)

@thamara thamara merged commit b5140f5 into thamara:master Jun 4, 2020
@thamara
Copy link
Owner

thamara commented Jun 4, 2020

Sorry everyone, I got confused and merged the PR into master.

@araujoarthur0
Copy link
Collaborator Author

Don't worry hahaha

@araujoarthur0 araujoarthur0 deleted the other-windows-dev-tools branch June 19, 2020 01:38
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