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

chore(ionic): update to v7 #29

Merged
merged 2 commits into from
Mar 29, 2023
Merged

chore(ionic): update to v7 #29

merged 2 commits into from
Mar 29, 2023

Conversation

brandyscarney
Copy link
Member

@brandyscarney brandyscarney commented Mar 20, 2023

  • updates to Ionic v7
  • does NOT update input syntax

@vercel
Copy link

vercel bot commented Mar 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
docs-demo ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 29, 2023 at 5:03PM (UTC)

@brandyscarney brandyscarney requested a review from a team March 20, 2023 21:33
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Mar 21, 2023

DOCS: It is not clear how swipeToClose becomes canDismiss for the card modal

In this case you can just remove swipeToClose since card modals are swipeable by default. If you wanted to prevent swiping then you can use canDismiss. I'll update the migration guide to clarify this.

edit: PR ionic-team/ionic-docs#2839

DOCS: Where is the documentation for the input syntax changes?

Input syntax changes are here: https://ionicframework.com/docs/v7/api/input#migrating-from-legacy-input-syntax

BUG: In the datetime example, in md mode, the bottom of the Location is cutoff (exists in v6 too)

If you use the new input syntax then the highlight goes away inside of ion-item.

liamdebeasi added a commit to ionic-team/ionic-docs that referenced this pull request Mar 21, 2023
Brandy noted in ionic-team/docs-demo#29 that it was not clear how to migrate from `swipeToClose` to `canDismiss`. This PR clarifies this on the migration guide.
@brandyscarney
Copy link
Member Author

Input syntax changes are here: https://ionicframework.com/docs/v7/api/input#migrating-from-legacy-input-syntax

Can we add this (and the checkbox, toggle, etc.) somewhere in the migration guide here: https://ionicframework.com/docs/v7/updating/7-0

There are warnings in the console about it but it's kind of hard to find these docs

@liamdebeasi
Copy link
Contributor

I don't think adding them to the migration guide is the right call here. The migration guide only notes the changes that developers need to make in order to have a working version of Ionic 7. The input syntax changes are optional at the moment, so devs don't need to make those changes.

Would it help if we linked to the docs from the console warning?

@brandyscarney
Copy link
Member Author

Would it help if we linked to the docs from the console warning?

Yeah that would be fine

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Code looks good. We don't need to make the input syntax changes right now, but if you have time and want to do it feel free.

@brandyscarney
Copy link
Member Author

I was going to do the input syntax changes but I am not sure how to approach some of them, like this:

<ion-item>
<ion-label position="stacked">Address</ion-label>
<ion-input placeholder="Address Line 1"></ion-input>
<ion-input placeholder="Address Line 2"></ion-input>
<ion-input placeholder="City"></ion-input>
<ion-input placeholder="State"></ion-input>
<ion-input placeholder="Zip Code"></ion-input>
</ion-item>

Want me to make a tech debt ticket for this and leave it for now?

@liamdebeasi
Copy link
Contributor

Sure you can make a tech debt ticket. For those inputs you'd want to add an aria-label to each and that should be sufficient.

@brandyscarney
Copy link
Member Author

Done: https://ionic-cloud.atlassian.net/browse/FW-3796

Is our plan to merge these immediately or wait until the 7.0 release?

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Mar 22, 2023

You can merge this now because the main deployment is configured to only show up on the v7 docs.

@brandyscarney
Copy link
Member Author

Okay do you want me to change the version to 7.0.0 for when it is released or wait?

@liamdebeasi
Copy link
Contributor

If you want to merge now then we'll need to wait on bumping to 7.0.0 otherwise the build will fail (since 7.0.0 is not out yet). Given how close we are to release, I'm fine just waiting to merge until release day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants