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

feat(native): add native settings section #1414

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

marekvospel
Copy link

@marekvospel marekvospel commented Jan 24, 2023

This pull request adds a settings section that is only shown in elk native app and minimize_to_tray configuration option.

Related

elk-zone/elk-native#58

@stackblitz
Copy link

stackblitz bot commented Jan 24, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Jan 24, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit 1c1dad0
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/6453e91d3b8d1200088c2d93

@netlify
Copy link

netlify bot commented Jan 24, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit 1c1dad0
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/6453e91d93c261000819174c
😎 Deploy Preview https://deploy-preview-1414--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@marekvospel marekvospel marked this pull request as ready for review January 24, 2023 20:51
@patak-dev
Copy link
Member

I wonder if we should add these options in elk-native itself, we should be able to add some hooks to extend the options 🤔
cc @danielroe @JonasKruckenberg

@danielroe
Copy link
Member

danielroe commented Jan 25, 2023

Yes. We could potentially also move the tauri module that exists in this repo -> elk native repo itself.

@JonasKruckenberg
Copy link
Contributor

I don't know how we would integrate this with the current submodule setup, but if you have any smart ideas, moving as much elk-native specific code into that repo sounds like a good solution to me generally

@marekvospel
Copy link
Author

I don't think it would be possible to move the tauri module into elk-native repository, as that could cause some type issues (just adding tauri-runtime-only types in this repository I had problems with types, see https://github.com/elk-zone/elk/blob/8bd02816203f5e00c7440689a877a5888107613a/modules/tauri/runtime/native-settings-undefined.ts)

At least I can't think of any good and easy solution

@Shinigami92 Shinigami92 added the c: feature Request for new feature label Feb 25, 2023
@marekvospel
Copy link
Author

I've resolved the merge conflicts. Should we wait till elk-zone/elk-native#65 is resolved before merging this?

I feel like for now we could just merge it and once it is decided whether we will continue to use git submodules or start using other methods, just migrate some of this code. I'll gladly migrate this into elk-native once and in case it is necessary.

@marekvospel
Copy link
Author

@JonasKruckenberg This method could be added to tauri-plugin-store to make it easier to work with the values reactively. However it is framework specific, so there could be some problems with dependencies and other framework integrations would probably need to be added.

async function tauriStoreRef<T>(store: Store, key: string, defaultValue: T): Promise<TauriStoreRef<T>> {
let refValue = defaultValue
const ref = customRef<T>((track, trigger) => {
store.onKeyChange(key, (value) => {
refValue = value as T
trigger()
}).then()
return {
get: () => {
track()
return refValue
},
set: async (value: T) => {
await store.set(key, value)
await store.save()
refValue = value
trigger()
},
}
})
const result = await store.get(key)
if (result !== undefined && result !== null)
refValue = result as T
return ref
}

@marekvospel
Copy link
Author

@Shinigami92 any chance for a review? This PR has been sitting here without any activity for a while

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

I could just do a code review
I don't use the native tauri app, you should ask someone else that is more familiar with the tauri app

modules/tauri/runtime/native-settings.ts Outdated Show resolved Hide resolved
modules/tauri/runtime/native-settings.ts Outdated Show resolved Hide resolved
modules/tauri/runtime/native-settings.ts Outdated Show resolved Hide resolved
modules/tauri/runtime/native-settings.ts Outdated Show resolved Hide resolved
modules/tauri/runtime/native-settings.ts Outdated Show resolved Hide resolved
modules/tauri/runtime/native-settings.ts Outdated Show resolved Hide resolved
@marekvospel
Copy link
Author

I could just do a code review I don't use the native tauri app, you should ask someone else that is more familiar with the tauri app

@JonasKruckenberg could you please take a look at this when you're free?

@@ -9,8 +9,10 @@ export default defineNuxtModule({
const nuxt = useNuxt()
const { resolve } = createResolver(import.meta.url)

if (!process.env.TAURI_PLATFORM)
if (!process.env.TAURI_PLATFORM) {
addImports({ name: 'useNativeSettings', from: resolve('./runtime/native-settings-undefined') })
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain why it is needed to have this -undefined version?
Cant this be e.g. inlined like addImports({ name: 'useNativeSettings', from: () => undefined }) or similar?

Copy link
Author

Choose a reason for hiding this comment

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

The reason I added the -undefined version is, that the non-undefined version cannot be imported unless in tauri environment.

It also serves the purpose of setting the types for the function (NativeSettings | undefined), because they wouldn't be generated in the prepare command, as the module returned before defining the global import.

I had no idea it was possible to import it inline, I'll take a look into it.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't find any way to add an import inline, the import object requires from: string value, and I couldn't find any other auto import releated function, that would do such a thing.

modules/tauri/runtime/native-settings.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature
Projects
Status: Awaiting Review
Development

Successfully merging this pull request may close these issues.

5 participants