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.

Sign up for GitHub

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

Jump to bottom

Replace Shortcut With KeyHolder #337

Closed
wants to merge 9 commits into from
Closed

Replace Shortcut With KeyHolder #337

wants to merge 9 commits into from

Conversation

AkaShark
Copy link
Collaborator

No description provided.

@AkaShark
feat: add keyholder 3rd package
7d9ad15
@AkaShark
feat: add shortcut section view
69dc6fb
@AkaShark
feat: add keyholder UI
4612e2c
@AkaShark
feat: setting shortcut UI finish
a6e649d
@AkaShark
Merge branch 'dev' into shortcut
32141ee 
# Conflicts:
#	Easydict.xcodeproj/project.pbxproj
#	Easydict.xcworkspace/xcshareddata/swiftpm/Package.resolved
@AkaShark
chore: merge error
419f851
@AkaShark
feat: keyholder wrapper
cea4cda
@AkaShark
Merge branch 'dev' into shortcut
62e6e9a 
# Conflicts:
#	Easydict.xcodeproj/project.pbxproj
#	Easydict/NewApp/View/SettingView/Tabs/GeneralTab.swift
@AkaShark
chore: merge
23c4a4c
@AkaShark
Copy link
Collaborator Author

I want to split GeneralTab.swift into some Views such as
CleanShot 2024-01-18 at 09 44 52@2x
CleanShot 2024-01-18 at 09 48 51@2x
but I am not familiar with swfitUI if there had any hidden dangers

@CanglongCl
Copy link
Collaborator

I want to split GeneralTab.swift into some Views such as CleanShot 2024-01-18 at 09 44 52@2x CleanShot 2024-01-18 at 09 48 51@2x but I am not familiar with swfitUI if there had any hidden dangers

This is a good practice and is also recommended.

CanglongCl
CanglongCl requested changes Jan 18, 2024
View reviewed changes
Easydict/NewApp/View/SettingView/Tabs/View/Genearl/GeneralOtherSetting.swift
}
}

@AppStorage(kHideMainWindowKey) private var hideMainWindow = false
Copy link
Collaborator

@CanglongCl CanglongCl Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can replace this with @Default(.hideMainWindow) private var hideMainWindow. Defaults package can provide global default value setting in UserDefaults management so it is recommended to use it instead of @AppStorage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay I got it

@tisfeng
Copy link
Owner

tisfeng commented Jan 18, 2024

@AkaShark Please don't push branch to main repository directly, you should push branch shortcut to your forked repo, and then open a PR, like this #335

image image

@AkaShark
Copy link
Collaborator Author

@AkaShark Please don't push branch to main repository directly, you should push branch shortcut to your forked repo, and then open a PR, like this #335

image image

okay sorry, I forget it... I will change the commit into my forked repo

@AkaShark AkaShark closed this Jan 18, 2024
@tisfeng
Copy link
Owner

tisfeng commented Jan 18, 2024

@AkaShark Please update code, move shortcuts code in Genearl to GeneralTab.swift and MenuItemView.swift, we need to test it in real use.

@AkaShark
Copy link
Collaborator Author

yeah I know it, I will open a new PR later

@tisfeng tisfeng deleted the shortcut branch January 18, 2024 14:57
@tisfeng
Copy link
Owner

tisfeng commented Jan 18, 2024

It's a minor issue, but in case we forget next time, I just added a protection rule to avoid this.

image

@Jerry23011 Jerry23011 changed the title [WIP] Replace Shortcut With KeyHolder Replace Shortcut With KeyHolder Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CanglongCl CanglongCl CanglongCl requested changes

@NeverAgain11 NeverAgain11 Awaiting requested review from NeverAgain11

@choykarl choykarl Awaiting requested review from choykarl

@tisfeng tisfeng Awaiting requested review from tisfeng

@Kyle-Ye Kyle-Ye Awaiting requested review from Kyle-Ye

@Jerry23011 Jerry23011 Awaiting requested review from Jerry23011

@phlpsong phlpsong Awaiting requested review from phlpsong

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AkaShark @CanglongCl @tisfeng