Feature/timetable feature fixes #48

charles-b-stb · 2024-06-15T03:27:45Z

Issue

close #ISSUE_NUMBER

Overview (Required)

Added better fake data
Adjusted layout to handle time blocks
Updated colors to better match other layouts and design

TODO

Still need to update tabs to highlight/de-highlight correctly
No live data connections yet
Scrolling also needs adjustment as it cuts off at the bottom

Links

Screenshot (Optional if screenshot test is present or unrelated to UI)

Before	After

Movie (Optional)

Before	After

… feature/timetable-feature-fixes

…djustments.

MrSmart00

Thx!!
I left some comments ✍️

MrSmart00 · 2024-06-15T10:45:49Z

app-ios/Sources/TimetableFeature/TimetableCore.swift

+    public var startsTimeString: String
+    public var endsTimeString: String
+    public var items: [TimetableItem]


These properties should be let.
because these don't change without constructor.

MrSmart00 · 2024-06-15T10:50:40Z

app-ios/Sources/TimetableFeature/TimetableCore.swift

+    // TODO: The actual class will look more like this
+//    public abstract val id: TimetableItemId
+//    public abstract val title: MultiLangText
+//    public abstract val startsAt: Instant
+//    public abstract val endsAt: Instant
+//    public abstract val category: TimetableCategory
+//    public abstract val sessionType: TimetableSessionType
+//    public abstract val room: TimetableRoom
+//    public abstract val targetAudience: String
+//    public abstract val language: TimetableLanguage
+//    public abstract val asset: TimetableAsset
+//    public abstract val levels: PersistentList<String>
+//    public abstract val speakers: PersistentList<TimetableSpeaker>


What is the meaning of these🤨?
Who are these comments for???
the production codes is not needed some comment-out codes I think🤔？

These were mostly for me. I can remove them.

This is based off of the existing object from the 2023 build. I was expecting to have to change to these objects later but I can just adjust to the object we get when the backend api is up and running.

app-ios/Sources/TimetableFeature/TimetableListView.swift

MrSmart00 · 2024-06-15T10:54:40Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

-            Text(tagText).foregroundColor (highlight ? Color.green : Color.gray)
-        }.padding(EdgeInsets(top: 2,leading: 7,bottom: 2,trailing: 7)).border(highlight ? Color.green : Color.gray).padding(-2)
+            Text(tagText).foregroundColor (highlight ? Color.green : Color(.onSurfaceColorset))
+        }.padding(EdgeInsets(top: 2,leading: 7,bottom: 2,trailing: 7)).border(highlight ? Color.green : Color(.onSurfaceColorset)).padding(-2)


I want linebreak plz🙏

MrSmart00 · 2024-06-15T10:59:49Z

app-ios/Sources/TimetableFeature/TimetableCore.swift

+    let startsAt: String
+    let endsAt: String


These should change to Date type I recommend.

I was expecting it to change to "instant" type but I can change to Date for now and swap out (or convert?) later.

MrSmart00 · 2024-06-15T11:06:07Z

app-ios/Sources/TimetableFeature/TimetableCore.swift

+        self.items = items
+    }
+}
+
 public struct TimetableItem: Equatable, Hashable {


The struct will needs convert from KMP right?
Such processing should be handled in a separate file, such as Middleware, so it seems better to separate this struct into another file.

Ok, I will move this do another file for now. Its likely to get replaced in the future either way.

Co-authored-by: Hiroya Hinomori <nexearth@gmail.com>

…gi/conference-app-2024 into feature/timetable-feature-fixes

charles-b-stb · 2024-06-16T14:06:05Z

@MrSmart00 Responded to your comments. Thanks!

MrSmart00 · 2024-06-16T14:12:42Z

app-ios/Sources/TimetableFeature/TimetableCore.swift


-        public init(timetableItems: [TimetableItem]? = []) {
+        public init(timetableItems: [TimetableTimeGroupItems]? = []) {


Sorry, I missed a comment for here🙏

Suggested change

public init(timetableItems: [TimetableTimeGroupItems]? = []) {

public init(timetableItems: [TimetableTimeGroupItems] = []) {

I think the optional setting is meaningless.

shin-usu · 2024-06-18T04:19:07Z

app-ios/Sources/TimetableFeature/TimetableCore.swift

After all this time, I think TimetableReducer would be an appropriate file name.

app-ios/Sources/TimetableFeature/TimetableListView.swift

shin-usu · 2024-06-18T04:27:48Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

@@ -78,8 +110,12 @@ struct TagView: View {
                Image(systemName: "diamond.fill").resizable().frame(width: 11,height: 11).foregroundColor(.green)
                    .padding(-3)
            }
-            Text(tagText).foregroundColor (highlight ? Color.green : Color.gray)
-        }.padding(EdgeInsets(top: 2,leading: 7,bottom: 2,trailing: 7)).border(highlight ? Color.green : Color.gray).padding(-2)
+            Text(tagText).foregroundColor(highlight ? Color.green : Color(.onSurfaceColorset))


Can green also be replaced by color set?

Co-authored-by: Usuda Shin <59346949+shin-usu@users.noreply.github.com>

…gi/conference-app-2024 into feature/timetable-feature-fixes

shin-usu · 2024-06-18T13:43:16Z

app-ios/Sources/TimetableFeature/SampleData.swift

+//
+//  File.swift
+//  
+//
+//  Created by CHARLES BOND on 2024/06/16.
+//
+


I think this header comment is unnecessary.

Sorry it was automatic

shin-usu · 2024-06-18T13:47:56Z

app-ios/Sources/TimetableFeature/SampleData.swift

+public struct SampleData {
+    public let day1Results = [


This sample data is only used in this module, right?
If so, I think that accessors should be at the internal level.

charles-b-stb · 2024-06-20T15:20:05Z

Responded to comments cc @MrSmart00 @shin-usu

shin-usu · 2024-06-23T08:53:31Z

app-ios/Sources/TimetableFeature/SampleData.swift

+    //TODO: This object is likely to change a lot when we get live data changes
+}
+
+internal struct SampleData {


Thank you for the correction!
By the way, Swift's default accessor is internal, so there is no need to explicitly describe it.

Suggested change

internal struct SampleData {

struct SampleData {

shin-usu · 2024-06-23T08:56:28Z

app-ios/Sources/TimetableFeature/SampleData.swift

+}
+
+internal struct SampleData {
+    public let day1Results = [


Please change this to the internal level, too🙏🏻
And please make same corrections to the other sample data.

shin-usu · 2024-06-23T08:58:58Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

@@ -15,7 +15,8 @@ public struct TimetableView: View {
                    Button(action: {
                        store.send(.selectDay(tabItem))
                    }, label: {
-                        Text(tabItem.rawValue).foregroundColor(.green)
+                        //TODO: Only selected button should be green and underlined
+                        Text(tabItem.rawValue).foregroundColor(Color(.greenSelectColorset))


There are still a few areas that are foregroundColor.
Please change to foregroundStyle.

MrSmart00

THX!!!
I left some comments 👍

MrSmart00 · 2024-06-23T09:21:01Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

+struct TimeGroupMiniList: View {
+    let contents: TimetableTimeGroupItems
+
+    public var body: some View {


Suggested change

public var body: some View {

var body: some View {

You should match the accessors to set them as default.👍

MrSmart00 · 2024-06-23T09:21:46Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

@@ -75,11 +107,15 @@ struct TagView: View {
    public var body: some View {


Suggested change

public var body: some View {

var body: some View {

MrSmart00 · 2024-06-23T09:22:02Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

@@ -90,15 +126,15 @@ struct PhotoView: View {

    public var body: some View {


Suggested change

public var body: some View {

var body: some View {

MrSmart00 · 2024-06-23T09:24:50Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

@@ -35,15 +38,41 @@ public struct TimetableListView: View {
    }

    public var body: some View {


This View class accessor should be default👍

Suggested change

public var body: some View {

var body: some View {

MrSmart00 · 2024-06-23T09:31:20Z

app-ios/Sources/TimetableFeature/TimetableReducer.swift

+        Reduce { state, action in
+            switch action {
+            case .onAppear:
+                state.timetableItems = SampleData.init().day1Results


SampleData should be a member property👍
ex)

public struct TimetableReducer { let sample = SampleData() ... public var body: some Reducer<State, Action> { Reducer{ state, action in switch action { case .onAppear: state.timetableItems = sample.day1Results ..

MrSmart00 · 2024-06-23T09:33:11Z

app-ios/Tests/TimetableTests/TimetableTests.swift

@@ -8,7 +8,7 @@ final class TimetableTests: XCTestCase {
            TimetableReducer()
        }
        await store.send(.onAppear) {
-            $0.timetableItems = SampleData.init().day1Data
+            $0.timetableItems = SampleData.init().day1Results


Suggested change

$0.timetableItems = SampleData.init().day1Results

$0.timetableItems = SampleData().day1Results

This is more like Swift👍

charles-b-stb · 2024-06-23T13:24:42Z

@MrSmart00 @shin-usu Responded to your comments, thanks!

MrSmart00

THX🙏
I left some comments!
check them please!

MrSmart00 · 2024-06-23T14:05:13Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

+            LazyVStack {
+                ForEach(store.timetableItems, id: \.self) { item in
+                    TimeGroupMiniList(contents: item)
+                }
+            }.scrollContentBackground(.hidden)
+
+                .onAppear {
+                    store.send(.onAppear)
+                }.background(Color(.backgroundColorset))


I think this style difficult for review...
you should alignment with indent.
could you fix them to below?

Suggested change

LazyVStack {

ForEach(store.timetableItems, id: \.self) { item in

TimeGroupMiniList(contents: item)

}

}.scrollContentBackground(.hidden)

.onAppear {

store.send(.onAppear)

}.background(Color(.backgroundColorset))

LazyVStack {

ForEach(store.timetableItems, id: \.self) { item in

TimeGroupMiniList(contents: item)

}

}

.scrollContentBackground(.hidden)

.onAppear {

store.send(.onAppear)

}

.background(Color(.backgroundColorset))

MrSmart00 · 2024-06-23T14:08:26Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

+        }.foregroundStyle(Color(.onSurfaceColorset)).padding(10)
            .overlay(
                RoundedRectangle(cornerRadius: 5)
-                    .stroke(Color.gray, lineWidth: 1)
+                    .stroke(Color(.onSurfaceColorset), lineWidth: 1)
            )


VStack(alignment: .leading) { ... } .foregroundStyle(Color(.onSurfaceColorset)).padding(10) .overlay( RoundedRectangle(cornerRadius: 5) .stroke(Color(.onSurfaceColorset), lineWidth: 1) )

shin-usu

LGTM👍👍

CHARLES BOND added 8 commits June 11, 2024 23:51

wip new data model (does not build)

e0d46d7

WIP (build broken again for unknown reasons)

f288219

Merge branch 'main' of github.com:DroidKaigi/conference-app-2024 into…

19ff20e

… feature/timetable-feature-fixes

updated test data

55b0945

Updated layout to handle date grouping properly. Still needs layout a…

1f3ea33

…djustments.

WIP (adding real colors)

7e7e144

Fixed basic colors

05a550e

Minor layout fixes

bd6d427

MrSmart00 reviewed Jun 15, 2024

View reviewed changes

charles-b-stb and others added 3 commits June 16, 2024 22:23

Update app-ios/Sources/TimetableFeature/TimetableListView.swift

a864ca1

Co-authored-by: Hiroya Hinomori <nexearth@gmail.com>

Addressed problems brought up in pull request review.

b4fdc46

Merge branch 'feature/timetable-feature-fixes' of github.com:DroidKai…

3c17479

…gi/conference-app-2024 into feature/timetable-feature-fixes

MrSmart00 reviewed Jun 16, 2024

View reviewed changes

Responded to pull request comments

a905d7a

shin-usu reviewed Jun 18, 2024

View reviewed changes

app-ios/Sources/TimetableFeature/TimetableListView.swift Outdated Show resolved Hide resolved

shin-usu reviewed Jun 18, 2024

View reviewed changes

charles-b-stb and others added 3 commits June 18, 2024 21:25

Update app-ios/Sources/TimetableFeature/TimetableListView.swift

5221c86

Co-authored-by: Usuda Shin <59346949+shin-usu@users.noreply.github.com>

responded to pull request comments.

30ddc0e

Merge branch 'feature/timetable-feature-fixes' of github.com:DroidKai…

d130c8e

…gi/conference-app-2024 into feature/timetable-feature-fixes

shin-usu reviewed Jun 18, 2024

View reviewed changes

pull request fixes.

d0bd648

charles-b-stb requested review from MrSmart00 and shin-usu June 20, 2024 15:19

shin-usu reviewed Jun 23, 2024

View reviewed changes

MrSmart00 reviewed Jun 23, 2024

View reviewed changes

Fixes to address concerns from pull request review.

14bbe2a

CHARLES BOND and others added 2 commits June 23, 2024 22:22

Fixed last pull request item.

d58c73d

Merge branch 'main' into feature/timetable-feature-fixes

fd4482b

charles-b-stb requested review from MrSmart00 and shin-usu June 23, 2024 13:24

MrSmart00 reviewed Jun 23, 2024

View reviewed changes

shin-usu approved these changes Jun 24, 2024

View reviewed changes

charles-b-stb merged commit faa8d59 into main Jun 26, 2024

charles-b-stb deleted the feature/timetable-feature-fixes branch June 26, 2024 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/timetable feature fixes #48

Feature/timetable feature fixes #48

charles-b-stb commented Jun 15, 2024

MrSmart00 left a comment

MrSmart00 Jun 15, 2024

MrSmart00 Jun 15, 2024

charles-b-stb Jun 16, 2024

MrSmart00 Jun 15, 2024

charles-b-stb Jun 18, 2024

MrSmart00 Jun 15, 2024

charles-b-stb Jun 16, 2024

MrSmart00 Jun 15, 2024

charles-b-stb Jun 16, 2024

charles-b-stb commented Jun 16, 2024

MrSmart00 Jun 16, 2024

shin-usu Jun 18, 2024

shin-usu Jun 18, 2024

shin-usu Jun 18, 2024

charles-b-stb Jun 20, 2024

shin-usu Jun 18, 2024

charles-b-stb commented Jun 20, 2024

shin-usu Jun 23, 2024

shin-usu Jun 23, 2024

shin-usu Jun 23, 2024

MrSmart00 left a comment

MrSmart00 Jun 23, 2024

MrSmart00 Jun 23, 2024

MrSmart00 Jun 23, 2024

MrSmart00 Jun 23, 2024

MrSmart00 Jun 23, 2024

MrSmart00 Jun 23, 2024

charles-b-stb commented Jun 23, 2024

MrSmart00 left a comment

MrSmart00 Jun 23, 2024

MrSmart00 Jun 23, 2024

shin-usu left a comment


		public init(timetableItems: [TimetableItem]? = []) {
		public init(timetableItems: [TimetableTimeGroupItems]? = []) {

		@@ -75,11 +107,15 @@ struct TagView: View {
		public var body: some View {

		@@ -90,15 +126,15 @@ struct PhotoView: View {

		public var body: some View {

		@@ -35,15 +38,41 @@ public struct TimetableListView: View {
		}

		public var body: some View {

	$0.timetableItems = SampleData.init().day1Results
	$0.timetableItems = SampleData().day1Results

Feature/timetable feature fixes #48

Feature/timetable feature fixes #48

Conversation

charles-b-stb commented Jun 15, 2024

Issue

Overview (Required)

TODO

Links

Screenshot (Optional if screenshot test is present or unrelated to UI)

Movie (Optional)

MrSmart00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-b-stb commented Jun 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-b-stb commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSmart00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-b-stb commented Jun 23, 2024

MrSmart00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shin-usu left a comment

Choose a reason for hiding this comment