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

Check your favorite sessions feature #40

Open
ewa1989 opened this issue Mar 18, 2024 · 7 comments
Open

Check your favorite sessions feature #40

ewa1989 opened this issue Mar 18, 2024 · 7 comments

Comments

@ewa1989
Copy link

ewa1989 commented Mar 18, 2024

I intended to create this issue according to contribution flow that first create an issue and then create a pull request.
If it's wrong please let me know.


Check your favorite sessions

feature written in README is yet to be released, isn't it?

I want to contribute to add this feature with multiple step below, because each step are viable and has some value.

  1. Only in ShceduleView, show star icons on the right inside of session row, toggle when the icons are tapped, and persist favorited states.
  2. In ScheduleView, add a filter to show only favorited sessions.
  3. In ScheduleDetailView, show a star icon on the right side of session title that can tap, and sync favorite state with ScheduleView.

If OK, I will create PR of step 1 first (already implemented in forked repository), and then try step 2.

@d-date
Copy link
Member

d-date commented Mar 18, 2024

Sounds good!

In ScheduleDetailView, show a star icon on the right side of session title that can tap, and sync favorite state with ScheduleView.

I think top right on the toolbar is better.

@ewa1989
Copy link
Author

ewa1989 commented Mar 18, 2024

Thank you!

I created a pull request of step 1.
#41

I tried step 2 from now, and

I think top right on the toolbar is better.

completely I agree!

And I have one more discussion about design, where and how to implement the favorited only filter.
It reminded me of SwiftUI Tutorials that adds a toggle as first item in List.
ref: https://developer.apple.com/tutorials/swiftui/handling-user-input#Add-a-control-to-toggle-the-state

What do you think of this implementation idea?

In current I implemented as ToolbarItem, but I thought it's better to follow Apple's app.

@d-date
Copy link
Member

d-date commented Mar 18, 2024

@ewa1989 I think filter icon like "line.horizontal.3.decrease" of SF Symbols is better.
And when tapping icon, pulldown is shown and selected from All or Favorite.
In this style, we can add more filters like "20 min" or "Lightning Talks" in the future.

Also I ask you to swap position with map icon since in the iPad, top leading icon will be put next to search bar.

@d-date
Copy link
Member

d-date commented Mar 18, 2024

Also ask you to add text, not only icon.

@ewa1989
Copy link
Author

ewa1989 commented Mar 18, 2024

@d-date

In this style, we can add more filters like "20 min" or "Lightning Talks" in the future.
Also I ask you to swap position with map icon since in the iPad, top leading icon will be put next to search bar.

Understandable example! I understand future plan!
Also this reason thinking of iPad is very learnful for me, thank you!
I came up with a better idea of code design.

Also ask you to add text, not only icon.

Does it mean showing appropriate text like "Filter" beside of "line.horizontal.3.decrease" of SF Symbols?

@d-date
Copy link
Member

d-date commented Mar 19, 2024

Does it mean showing appropriate text like "Filter" beside of "line.horizontal.3.decrease" of SF Symbols?

Right!

@ewa1989
Copy link
Author

ewa1989 commented Mar 20, 2024

@d-date
I completed fixing as reviewed #41. and tried step 2 and 3 and create PRs in my forked repository.
- PR of step 2
- PR of step 3
Because of so much changes in step 1, I'll re-create branches of step 2 and 3 for readability.

If you have time after re-reviewing #41, could you do code review them?
In current PRs are in my forked repository for making it easy to understand diffs but I will re-create them in this repository after merging #41.

Sorry for taking much time under condition of few time until the conference. 🙇

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

No branches or pull requests

2 participants