1647: Add store links #1708
1647: Add store links #1708
Conversation
…er to separate component to reuse it
f1sh1918
requested review from
steffenkleinle,
ztefanie and
seluianova
as code owners
| <StoreLink href={appStoreUrl} target='_blank' rel='noreferrer'> | ||
| <StoreIcon src={AppleStoreIcon} alt='AppStore öffnen' /> |
There was a problem hiding this comment.
🔧 I think it would be cool to keep the naming consistent, either app store or apple store all the time. Same for android store vs play store.
There was a problem hiding this comment.
For google they always use "Google Play öffnen" and for Apple "App Store öffnen"
This is a common thing and branded on the icons
There was a problem hiding this comment.
Sorry for the misunderstanding, this comment is about variable naming, not about the user facing texts.
We have link vs url and appStore vs appleStore.
administration/src/bp-modules/self-service/CardSelfServiceInformation.tsx
Outdated
Show resolved
Hide resolved
| @@ -104,11 +104,13 @@ const bayern: BuildConfigType = { | |||
| excludeLocationPlayServices: false, | |||
| excludeX86: false, | |||
| }, | |||
| appStoreLink: `https://play.google.com/store/apps/details?id=${ANDROID_APPLICATION_ID}`, | |||
There was a problem hiding this comment.
🙃 : As this is redundant to the applicationId, I think it would be nice to just construct it from the applicationId in AppStoreLinks and not any additional property here.
| appStoreLink: `https://play.google.com/store/apps/details?id=${ANDROID_APPLICATION_ID}`, |
There was a problem hiding this comment.
So you think i should create a constant for the base appStoreLink like "https://play.google...." and then construct it with the applicationId.
Hm for me the issue is that for the app store link (apple) its constructed very different and i would need to also set a base url like https://apps.apple.com/de/app/
and then use the application name and id ehrenamtskarte-bayern/id1261285110
If i do that i would have to create a separate Id for apple and export it to be able to construct the complete appStoreLink in a common way in the AppStoreLinks
I don't think that its worth the effort tbh
| }, | ||
| ios: { | ||
| ...bayernCommon, | ||
| bundleIdentifier: IOS_BUNDLE_IDENTIFIER, | ||
| provisioningProfileSpecifier: "match AppStore de.nrw.it.ehrensachebayern", | ||
| appStoreLink: "https://apps.apple.com/de/app/ehrenamtskarte-bayern/id1261285110", |
There was a problem hiding this comment.
🙃 Optional: Personally I think it might be better to also just add the appStoreId to the build configs, e.g. id1261285110 in this case.
This would
- allow us to use the appStoreId separately for e.g. an
apple-itunes-appmeta tag for websites - allow us to set the language of the link dynamically based on the users language
- reduce the chance of copy/paste mistakes like setting the language to
ky
However, due neither of the first two points being relevant to us atm, feel free to just keep this as it is.
There was a problem hiding this comment.
hm same as i mentioned above. the i also need thisehrenamtskarte-bayern separately.
I don't see a big improvement here and will keep my solution
|
may i get a second review @seluianova ? :) |
|
Is a 'zu' needed here? |
Short description
For the download instructions of the koblenz pass self service we need store links. in #1707 we may also create a separate app download route and need also the urls for the other projects
Proposed changes
AppStoreLinkscomponent to make it reusable for Replace download app page #1707Side effects
-none
Testing
npm iran on root folder to have new buildConfig variablesResolved issues
Fixes: #1647