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

refactor menu to use constants #1087

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evanm510
Copy link

@evanm510 evanm510 commented Dec 5, 2024

Related issue

https://github.com/thamara/time-to-leave/issues/1085

Context / Background

Currently, menu labels in the menu.js file are hardcoded across multiple functions, which makes the code harder to maintain and increases the chance of inconsistencies. Updating a menu label or shortcut requires finding each occurrence, making changes more time-consuming and prone to error. This PR introduces a set of centralized constants for menu labels. By using constants, menu items can be updated in one place, improving maintainability, readability, and reducing redundancy across the codebase.

What change is being introduced by this PR?

  • Introduced a MenuConstants object at the top of menu.js to store all menu labels.
  • Refactored all menu templates (getMainMenuTemplate, getContextMenuTemplate, etc.) to replace hardcoded strings with references to MenuConstants.

How will this be tested?

  • Manually verify that all menus load as expected and perform their respective actions (e.g., opening preferences, exporting databases).
  • Check that menu labels match the original functionality.

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.

Some places were changed without any reasoning, so I mostly highlighted those

Comment on lines +384 to +385
getCurrentTranslation(MenuConstants.Labels.YES),
getCurrentTranslation(MenuConstants.Labels.NO)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changed from "Copy" and "ok" to "Yes" and "no". Please confirm it was intentional and if so, this should be on a separate commit, as it is a bug fix.

const options = {
title: getCurrentTranslation('$Menu.export-db-to-file'),
title: getCurrentTranslation(MenuConstants.Labels.EXPORT_DATABASE),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This used to be $Menu.export-db-to-file, not $Menu.export-database

defaultPath: `time_to_leave_${getCurrentDateTimeStr()}`,
buttonLabel: getCurrentTranslation('$Menu.export'),

buttonLabel: getCurrentTranslation(MenuConstants.Labels.EXPORT_DATABASE),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This used to be $Menu.export, not $Menu.export-database

Comment on lines +231 to +232
title: getCurrentTranslation(MenuConstants.Labels.IMPORT_DATABASE),
buttonLabel: getCurrentTranslation(MenuConstants.Labels.IMPORT_DATABASE),
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changed similar to the export

exportDatabaseToFile(response);
dialog.showMessageBox(BrowserWindow.getFocusedWindow(), {
title: 'Time to Leave',
message: getCurrentTranslation('$Menu.database-export'),
message: getCurrentTranslation(MenuConstants.Labels.DATABASE_WAS_EXPORTED),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Used to be database-export not database-was-exported

dialog.showMessageBox(BrowserWindow.getFocusedWindow(), {
title: 'Time to Leave',
message: getCurrentTranslation('$Menu.database-imported'),
message: getCurrentTranslation(MenuConstants.Labels.DATABASE_IMPORTED),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same change as in export.

} ${getCurrentTranslation('$Menu.could-not-be-loaded')}`;
} else if (importResult['failed'] !== 0) {
const message = `${importResult['failed']}/${importResult['total']} ${getCurrentTranslation(
MenuConstants.Labels.FAILED_ENTRIES
Copy link
Collaborator

Choose a reason for hiding this comment

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

Used to be could-not-be-loaded

mainWindow.webContents.send('RELOAD_CALENDAR');
dialog.showMessageBox(BrowserWindow.getFocusedWindow(), {
title: 'Time to Leave',
message: getCurrentTranslation('$Menu.clear-database'),
message: getCurrentTranslation(MenuConstants.Labels.ALL_CLEAR),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Used to be clear-database

Comment on lines +298 to +299
getCurrentTranslation(MenuConstants.Labels.NO),
getCurrentTranslation(MenuConstants.Labels.YES)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dropped the cancel button and is listing buttons in a different order

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