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

[Update] Sample Viewer category front page #221

Merged
merged 27 commits into from
Jul 18, 2023
Merged

Conversation

CalebRas
Copy link
Contributor

@CalebRas CalebRas commented Jul 6, 2023

Description

This PR updates the front page of the sample viewer to organize the samples into categories, similar to the old iOS Sample Viewer. The new common design categories were used. All of the currently implemented samples fall under one of the sample categories.

Linked Issue(s)

  • swift/issues/4229

How To Test

  • Load the sample viewer.
  • Tap on a category to navigate to a list of samples in that category.
  • Tap on the search bar to search for a sample.

Screenshots

Before After
Simulator Screen Shot - iPhone 14 Pro - 2023-07-06 at 15 49 35 Simulator Screen Shot - iPhone 14 Pro - 2023-07-06 at 15 43 26

To Discuss

  • There is a bug where the navigation title at the top of the samples list jumps after tapping on a category and the list first loads.
  • There is a bug when you back out of a sample after you entered it through a category. It then loads to the category view instead of the samples list view and it no longer lets you in that specific category.

@CalebRas CalebRas requested a review from yo1995 July 6, 2023 23:09
@CalebRas CalebRas self-assigned this Jul 6, 2023
@CalebRas CalebRas marked this pull request as ready for review July 7, 2023 00:05
Copy link
Collaborator

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

I'm not particularly good at naming views, so let's discuss that with other reviewers in the next step.

First batch of comments.

@CalebRas CalebRas requested a review from yo1995 July 13, 2023 17:45
Samples.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
Samples.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
Shared/Supporting Files/Views/CategoryGridView.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Views/CategoryGridView.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Views/CategoryGridView.swift Outdated Show resolved Hide resolved
@CalebRas CalebRas requested a review from yo1995 July 13, 2023 21:09
Copy link
Collaborator

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

LGTM

Shared/Supporting Files/Views/CategoryGridView.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Views/CategoryGridView.swift Outdated Show resolved Hide resolved
@yo1995 yo1995 requested a review from a team July 13, 2023 23:35
@yo1995 yo1995 requested review from des12437 and dfeinzimer and removed request for a team July 13, 2023 23:35
@yo1995
Copy link
Collaborator

yo1995 commented Jul 13, 2023

I added David to the reviewers as he is familiar with UI best practices.

@yo1995
Copy link
Collaborator

yo1995 commented Jul 13, 2023

There is a bug when you back out of a sample after you entered it through a category. It then loads to the category view instead of the samples list view and it no longer lets you in that specific category.

I read the code and couldn't find anything that might caused this bug, except the difference between iOS 16 NavigationSplitView and iOS 15 NavigationView. I also tested on iOS 15.5 and the bug is not there 🤔 . @philium If you have a chance can you help take a look at CategoryGridView and see if anything stands out? Thank you.

CalebRas and others added 3 commits July 14, 2023 15:24
Co-authored-by: David Feinzimer <dfeinzimer@gmail.com>
Co-authored-by: David Feinzimer <dfeinzimer@gmail.com>
@CalebRas CalebRas requested a review from dfeinzimer July 17, 2023 22:48
Copy link
Contributor

@des12437 des12437 left a comment

Choose a reason for hiding this comment

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

Looks good. I'm not sure if the AboutView is within the scope of this PR, but noticed that the logo and copyright text could use some spacing.

The copyright year can also be generated rather than hardcoded:

Text("Copyright © 2022 - \(String(Calendar.current.component(.year, from: Date()))) Esri. All Rights Reserved.")

Shared/Supporting Files/Views/CategoryView.swift Outdated Show resolved Hide resolved
Shared/Supporting Files/Views/CategoryView.swift Outdated Show resolved Hide resolved
Co-authored-by: David Feinzimer <dfeinzimer@gmail.com>
Co-authored-by: Destiny Hochhalter <117859673+des12437@users.noreply.github.com>
@CalebRas
Copy link
Contributor Author

Looks good. I'm not sure if the AboutView is within the scope of this PR, but noticed that the logo and copyright text could use some spacing.

The copyright year can also be generated rather than hardcoded:

Text("Copyright © 2022 - \(String(Calendar.current.component(.year, from: Date()))) Esri. All Rights Reserved.")

Yea I think this would fall under another issue/PR.

@yo1995
Copy link
Collaborator

yo1995 commented Jul 18, 2023

Please see Phil's comment on the second part of the year string: #3 (comment) It shouldn't be dynamic, but rather manually updated to reflect the last change to the code.

@CalebRas CalebRas merged commit a9ef1a9 into v.next Jul 18, 2023
1 check passed
@CalebRas CalebRas deleted the Caleb/Update-CategoriesUI branch July 18, 2023 21:38
@yo1995
Copy link
Collaborator

yo1995 commented Jul 18, 2023

Sorry, I meant to hold this PR from being merged until the navigation bug (mentioned in the "To Discuss" section in the description) is addressed. I'll let Phil to comment on that.

@dfeinzimer and @des12437 do you have thought on that?

@dfeinzimer
Copy link
Contributor

dfeinzimer commented Jul 18, 2023

@dfeinzimer and @des12437 do you have thought on that?

Hm, sorry I missed those in the description!

Neither of them seem to still be present? Am I missing? No jumping spotted and I'm able to re-open the Analysis category.

2023-07-18 15 28 08

That's unless you mean the jumping the second time around when coming back into the Analysis category?

@yo1995
Copy link
Collaborator

yo1995 commented Jul 18, 2023

@dfeinzimer That's intriguing behavior! 😂 May I ask which Xcode and Sim version are you using? It differs from mine both on iOS 15 and 16…

That's unless you mean the jumping the second time around when coming back into the Analysis category?

When you tap "Back" from a sample, it directly jump back to the category grid view, which means sth's wrong with the navigation stack. 🤔

@dfeinzimer
Copy link
Contributor

May I ask which Xcode and Sim version are you using? It differs from mine both on iOS 15 and 16…

Xcode 14.3.1 with a 14 Pro Max sim on iOS 17.0

@des12437
Copy link
Contributor

@dfeinzimer and @des12437 do you have thought on that?

I see the same navigation issue on Xcode 14.3.1 and iPhone 14 Pro Max simulator iOS 16.4.

philium added a commit that referenced this pull request Jul 20, 2023
Fixes issue described [here](#221 (comment)), where going back from a sample takes the user all the way back to the Categories view, not the view for the category to which the sample belongs. This also means that when the categories are displayed in a sidebar, tapping on a category opens that category in the sidebar, which is expected behavior for this kind of a navigation setup on iOS.
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.

None yet

4 participants