feat: add 'Starting from weekday' date range#10
feat: add 'Starting from weekday' date range#10Raian256 wants to merge 1 commit intodougcooper:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the dashboard's date range functionality by adding a new preset option. Users can now easily define a date range that begins from a specific day of the week, relative to the current date. This provides more flexibility in data visualization and reporting without requiring manual date input. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new "Starting from weekday" date range preset to the dashboard. This feature includes UI updates in sp-dashboard/index.html to add the new preset option and a corresponding weekday selection dropdown, along with JavaScript logic to manage their visibility and calculate the date range. A new test case in tests/index.test.js validates this functionality. Review feedback highlights that the new test for the "from-weekday" preset could be flaky due to its reliance on new Date() and suggests mocking system time for determinism. Additionally, it's recommended to cache the weekday-select DOM element reference for improved performance and maintainability, and to remove a redundant window.processData call in the new test.
| expect(weekdayPickerContainer.classList.contains('hidden')).toBe(false); | ||
|
|
||
| // Test each weekday | ||
| const today = new Date(); |
There was a problem hiding this comment.
This test could be flaky because it relies on the current system time (new Date()). If the test execution crosses a day boundary between this line and the new Date() call inside processData, the test might fail unexpectedly. It's a best practice to mock date/time for such tests to ensure they are deterministic.
You can use vi.useFakeTimers() and vi.setSystemTime() to control the system time within your test. For example:
const mockToday = new Date('2024-01-26T12:00:00Z'); // A Friday
vi.useFakeTimers();
vi.setSystemTime(mockToday);
// ... your test logic ...
vi.useRealTimers(); // cleanup| } else if (preset === 'year') { | ||
| startObj.setFullYear(endObj.getFullYear() - 1); | ||
| } else if (preset === 'from-weekday') { | ||
| const targetDay = parseInt(document.getElementById('weekday-select').value, 10); |
There was a problem hiding this comment.
For better performance and maintainability, it's a good practice to cache DOM element references instead of querying for them repeatedly. The weekday-select element is accessed here and also when attaching an event listener (line 541). You can get the element once and store it in a variable at the top of the script block, similar to how presetSelect and customContainer are handled.
For example, you could add this line near line 533:
const weekdaySelect = document.getElementById('weekday-select');Then you would change line 541 to:
weekdaySelect.addEventListener('change', () => processData(cachedTasks, cachedProjects));And this line would become:
| const targetDay = parseInt(document.getElementById('weekday-select').value, 10); | |
| const targetDay = parseInt(weekdaySelect.value, 10); |
| for (const targetDay of [0, 1, 2, 3, 4, 5, 6]) { | ||
| weekdaySelect.value = String(targetDay); | ||
| weekdaySelect.dispatchEvent(new Event('change')); | ||
| window.processData([], []); |
There was a problem hiding this comment.
The dispatchEvent on the previous line already triggers processData via the event listener. This explicit call to window.processData is redundant and can be removed.
1 participant
Summary
Adds a new "Starting from weekday" date preset. When selected, a
secondary dropdown appears to pick a specific weekday (Mon–Sun).
The date range then spans from the most recent occurrence of that
weekday up to today (inclusive).
Example: today is Thursday → pick Tuesday → 3-day range.
Changes
index.html— new preset option, weekday picker UI, show/hidelogic, and date calculation
tests/index.test.js— test covering picker visibility andcorrect range length for all 7 weekdays