Skip to content

Add preset time dropdowns for work, rest, and loop durations #134

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

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

Conversation

Dilemma-CMZ
Copy link

Add preset time dropdowns for work, rest, and loop durations

截屏2025-04-20 21 00 33

Overview

This PR introduces a new user interface improvement to wnr that allows users to quickly select common time durations through dropdown menus with a scrollable interface, rather than manually typing values each time.

Features

  • Added preset time dropdowns for work sessions, rest sessions, and loop counts
  • Implemented a scrollable dropdown menu for better UX when many presets are available
  • Styled dropdowns to match the existing application design language
  • Color-coded dropdowns to match their respective functions (work=red, rest=blue, onlyRest=purple)
  • Added hover effects for better visual feedback

UI Changes

  • Small dropdown toggle buttons next to time input fields
  • Dropdown menus appear on click with preset time values
  • Scrollable dropdown menus that can accommodate multiple options
  • Consistent styling with the rest of the application

@RoderickQiu RoderickQiu self-requested a review April 21, 2025 03:15
@RoderickQiu
Copy link
Owner

Two issues to mention:

  1. Must fix: it does not support multi-languages.
  2. Shall better fix: the scroll-down list does not support dark mode.

…ple languages and enhance dropdown item display for time presets
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
7.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@Dilemma-CMZ
Copy link
Author

I have fixed the problem and it could now support multi-language.

@RoderickQiu
Copy link
Owner

Feedbacks:

  1. Now your new commits introduce new bugs. If you click option+command+i will see bugs happening.
  2. And your way of localization is not preferred, that it hard-coded strings inside the code. All strings referred must only be located in locales/*.json and you must have no redundant strings inside it. And although that is not the case right now, but let's imagine we are having the fourth type of localization support and your current code will not work in this way.
  3. In theme.js, you just created new darkmode support, which is redundant. Look at the code below you will find that.
    If you use AI, you must recheck that you are not introducing unnecessary changes and revert them using your git.

// For Chinese languages, use specific characters
const currentLang = store.get('i18n') || 'en';
if (currentLang === 'zh-CN') {
minutes = '分钟';
Copy link
Owner

Choose a reason for hiding this comment

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

Hard-coded strings like this is not preferred and does not support fourth locale support even if there is not such a thing as for now.

@@ -394,5 +394,6 @@
"can-redo-msg": "是否需要对当前段时间进行重新计时?",
"custom": "自定义",
"custom-notify-sound": "自定义提示音",
"input-url": "输入路径"
"input-url": "输入路径",
"minutes": "minutes-[zh-CN]"
Copy link
Owner

Choose a reason for hiding this comment

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

Redundant string is not OK for all three language jsons.

})();

// Apply custom colors to dropdown elements
$('body').append(
Copy link
Owner

Choose a reason for hiding this comment

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

If you search in all code base you will find that dark mode is already supported in somewhere else so you shall not implement it again.

if (!i18n.__('minutes')) {
// Define a mapping for full words based on language
const minutesMapping = {
'zh-CN': '分钟', // Simplified Chinese
Copy link
Owner

Choose a reason for hiding this comment

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

Also redundant here. You shall never hard-code anything but should always refer from locales/*.json using i18n.__("") syntax.

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.

2 participants