Skip to content
This repository has been archived by the owner on May 2, 2024. It is now read-only.

ios splash screen #4

Merged
merged 17 commits into from
May 30, 2022
Merged

Conversation

xlanex6
Copy link
Contributor

@xlanex6 xlanex6 commented May 23, 2022

No description provided.

@xlanex6
Copy link
Contributor Author

xlanex6 commented May 23, 2022

@kevinmarrec

Feedback could be great, as I told you , I'am rookie on this, but trust my motivation

lib/splash.cjs Outdated Show resolved Hide resolved
src/module.ts Outdated Show resolved Hide resolved
lib/splash.cjs Outdated Show resolved Hide resolved
@xlanex6 xlanex6 requested a review from kevinmarrec May 24, 2022 07:13
Copy link
Owner

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

Added some comments about improvements & eventual fixes.

I may iterate on new comments afterwards so we can polish this feature 🚀

src/meta.ts Outdated Show resolved Hide resolved
src/meta.ts Outdated Show resolved Hide resolved
lib/splash.cjs Outdated Show resolved Hide resolved
lib/splash.cjs Outdated Show resolved Hide resolved
src/module.ts Show resolved Hide resolved
@xlanex6 xlanex6 requested a review from kevinmarrec May 24, 2022 14:47
@xlanex6 xlanex6 changed the title Draft : ios splash screen ios splash screen May 24, 2022
Copy link
Owner

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

Thanks for the updates !

Could you take into account the 2 unresolved comments from my last review ? backgroundColor & extra comma

I added a comment that you can commit right away (it's just about removing useless string interpolation for href)

And finally, I think that we could refactor everything to just give devices to an unique lib/resize.cjs that would generate the splash screens aside the first icons. (We could think about a splash object as param with splash.devices array, so we can have latersplash.backgroundColor and more options in the future). This would remove the redundancy of the 2 files present in lib.

src/meta.ts Outdated Show resolved Hide resolved
@kevinmarrec kevinmarrec added the enhancement New feature or request label May 24, 2022
@kevinmarrec kevinmarrec modified the milestone: v0.2.0 May 24, 2022
@xlanex6
Copy link
Contributor Author

xlanex6 commented May 24, 2022

@kevinmarrec

I made a choice to set devices array as default module's options.
Kept all devices by default, is it good ?

I add also some types definition due about splash key introduction in Icon

Question :

Is it make sens to keep splash-screen generation triggers by manifest.mobileAppIOS if we provide splash object in icon ?
should we splash: { devices , .....} or splash : false to prevent generation ?

@xlanex6 xlanex6 requested a review from kevinmarrec May 24, 2022 20:30
@kevinmarrec
Copy link
Owner

kevinmarrec commented May 25, 2022

@xlanex6 I think we could use icon.iosSizes to keep aligned with Nuxt 2 PWA for now.

But as Nuxt 3 modules uses defu which merges configuration arrays, we need to do like https://github.com/kevinmarrec/nuxt-pwa-module/blob/main/src/icon.ts#L32 to only set icon.iosSizes to default devices if the array is empty (so that people can still override it)

Then, in both icon & meta sub modules, we should check pwa.meta.mobileAppIOS to decide if the icon generation & meta inject are needed.

So if i'm right, these wouldn't be generated by default, but everything could be opt in with mobileAppIOS: true

@xlanex6
Copy link
Contributor Author

xlanex6 commented May 25, 2022

@kevinmarrec

Based on your previous comment
I implement devices array as empty by default , and inject if user not overwrite it. ( same pattern )

splash screen generation still trigged by mobileAppIOS: true ( previous commit )

lib/splash.cjs Outdated Show resolved Hide resolved
lib/splash.cjs Outdated Show resolved Hide resolved
src/meta.ts Outdated Show resolved Hide resolved
src/meta.ts Outdated Show resolved Hide resolved
lib/splash.cjs Outdated Show resolved Hide resolved
src/module.ts Show resolved Hide resolved
src/module.ts Show resolved Hide resolved
@kevinmarrec
Copy link
Owner

@xlanex6 Just letting you know that I'll have a look on last updates only Sunday, as I'm off computer until this day :)

@kevinmarrec
Copy link
Owner

kevinmarrec commented May 30, 2022

@xlanex6 Do you mind if I merge the current state in a splash branch here ?

I'd like to have it released by tomorrow evening, but I still have changes to request (most likely minors).

When merged in main, we'll be both co-authors of the commit so we track your contribution, what do you think ?

@kevinmarrec
Copy link
Owner

We could also just wait next week, as if there's any issue with the release I won't really be able to work on it this week (I'm off computer most days of this week)

@xlanex6
Copy link
Contributor Author

xlanex6 commented May 30, 2022

@kevinmarrec

Both options are good for me until my tiny contribution still there.

@kevinmarrec kevinmarrec changed the base branch from main to splash May 30, 2022 14:37
@kevinmarrec kevinmarrec merged commit 7b2c8a9 into kevinmarrec:splash May 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants