-
Notifications
You must be signed in to change notification settings - Fork 3.2k
docs(vue): show complete code context in the "Your First App" tutorial #4197
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
base: main
Are you sure you want to change the base?
Conversation
…t in your first app page 1
|
@soundproofboot is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that some method names from the Angular version and Vue version do not match even though they have the same code. For example, addNewToGallery from Angular and usePhotoGallery from Vue. Let's also sync those to match Angular as close as possible.
The same goes for the <title>. Let's match it to Angular.
Lastly, some of the content also doesn't match each other. A good example, Angular Build Your First App mentions:
We put the visual aspects of our app into <ion-content>. In this case, it’s where we’ll add a button that opens the device’s camera as well as displays the image captured by the camera. Start by adding a [floating action button](https://ionicframework.com/docs/api/fab) (FAB) to the bottom of the page and set the camera image as the icon.
While Vue states:
We put the visual aspects of our app into <ion-content>. In this case, it’s where we’ll add a button that opens the device’s camera as well as displays the image captured by the camera. But first, remove both the ExploreContainer component and its import statement:
It should match the steps of Angular.
Co-authored-by: Maria Hutt <[email protected]>
…h new Angular docs
|
@thetaPC Hi Maria, I tried to make the titles match in Vue and added some On making the steps match Angular, there are more steps required for Vue in that specific case so I think they are slightly reordered to clarify what's being removed and added in Vue (as in remove these two things before we add these other several things) vs. Angular, where I was able to clearly indicate what to remove and add in one code block. I have not looked at this in a while so it may take me a minute to familiarize myself with the frameworks and changes but I'm willing to keep working on this if there are other suggestions. |
|
@soundproofboot thank you so much! To ensure that all 'Your First App' PRs are merged more quickly, I will be assisting to push them to completion. The main obstacle is synchronizing all three frameworks. Please bear with me as I make the necessary changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested some changes on v8 that will also apply to v7. 🙂
| Next, create an Ionic Vue app that uses the "Tabs" starter template and adds Capacitor for native functionality: | ||
|
|
||
| ```shell | ||
| ionic start photo-gallery tabs --type vue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does Angular pass --capacitor but we don't here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly not sure. This command was here before the edits so I figured that there was a reason for it. Do you think we can remove it from Angular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI sets it automatically for all of them (except if cordova is passed for Angular), so it looks like we can remove it for Angular and React too:
if (this.schema.type === 'react' || this.schema.type === 'vue') {
options['capacitor'] = true;
}
if (
(this.schema.type === 'angular' || this.schema.type === 'angular-standalone')
&& options['cordova'] === null
) {
options['capacitor'] = true;
}
docs/vue/your-first-app.md
Outdated
|
|
||
| ```html | ||
| ```vue | ||
| <ion-title>Photo Gallery</ion-title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be confusing if people interpret it to change the large title to this:
-<ion-title size="large">Tab 2</ion-title>
+<ion-title>Photo Gallery</ion-title>I realize this is the same as the Angular example, just pointing it out because I didn't notice it until this review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the change for Angular: c19dbea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the v7 version for vue got missed: https://github.com/ionic-team/ionic-docs/pull/4197/files#diff-caf0d45c0315d4fb3d8a52d48018965b3d06ba16af64aad7496ad5c99f3a9a64R174
| IonGrid, | ||
| IonRow, | ||
| IonCol, | ||
| IonImg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was leaning towards showing them as they get added but I ended up working on this too much that I decided to leave it in as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm okay. I think it would be a better user experience if they didn't have errors in their IDE while going through the steps. Maybe we can make a follow-up task for this?
| When running on mobile, set `filepath` to the result of the `writeFile()` operation - `savedFile.uri`. When setting the `webviewPath`, use the special `Capacitor.convertFileSrc()` method ([details on the File Protocol](../../core-concepts/webview.md#file-protocol)). To use this method, we'll need to import Capacitor into `usePhotoGallery.ts`. | ||
|
|
||
| ```ts | ||
| import { Capacitor } from '@capacitor/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| Then update `savePicture()` to look like the following: | ||
|
|
||
| ```ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this from a broader perspective, I think this example is much easier to follow. When we're updating an existing function, it makes more sense to show only that function. When we're adding a new function, then it's helpful to include the surrounding code for context.
In other examples we show the surrounding code even when nothing in it changes, but that isn't necessary if the updates apply only to one function:
(Above screenshot taken from Platform-specific Logic section)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, the reason I did this was because the examples with the surrounding code were shown at the beginning of the tutorial. As the developer progresses, I expect them to feel comfortable with what they've done so we don't need to have the surrounding code as they would know where in the file is the update function. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't think I was clear. I like that you are only showing the function itself and not the surrounding code when the changes are limited to that function. I was just saying, in general, I think we should do it this way in more places. It's less confusing.
Co-authored-by: Brandy Smith <[email protected]>
docs/vue/your-first-app.md
Outdated
|
|
||
| ```html | ||
| ```vue | ||
| <ion-title>Photo Gallery</ion-title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the v7 version for vue got missed: https://github.com/ionic-team/ionic-docs/pull/4197/files#diff-caf0d45c0315d4fb3d8a52d48018965b3d06ba16af64aad7496ad5c99f3a9a64R174
| IonGrid, | ||
| IonRow, | ||
| IonCol, | ||
| IonImg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm okay. I think it would be a better user experience if they didn't have errors in their IDE while going through the steps. Maybe we can make a follow-up task for this?
| When running on mobile, set `filepath` to the result of the `writeFile()` operation - `savedFile.uri`. When setting the `webviewPath`, use the special `Capacitor.convertFileSrc()` method ([details on the File Protocol](../../core-concepts/webview.md#file-protocol)). To use this method, we'll need to import Capacitor into `usePhotoGallery.ts`. | ||
|
|
||
| ```ts | ||
| import { Capacitor } from '@capacitor/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Brandy Smith <[email protected]>


resolves #4165
Changes:
Preview