Feature/timetable feature grid2 #367

charles-b-stb · 2024-08-12T15:35:53Z

Issue

??

Overview (Required)

Added very basic gridview to iOS timetable
Will need additional fixes before full release

Links

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

Before	After

Movie (Optional)

Before	After
	Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-13.at.00.25.11.mp4

… feature/timetable-feature-grid2

…ll need to space welcometalk/lunch/etc.

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

ry-itto · 2024-08-13T11:15:38Z

app-ios/Sources/TimetableFeature/SampleData.swift

+        }.first
+    }
+
+    func getCellForRoom(room: RoomType, onTap: @escaping (TimetableItem) -> Void) -> TimetableGridCard {


This is the view code. Move this to the appropriate view definition file

Moved to the UI file. I still am using an extension because if the code in the loop gets too complicated iOS just fails to render the list.

If you have a better suggestion for this I am open for other ideas. 🙇

ry-itto · 2024-08-13T11:16:52Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

+    var body: some View {
+        let rooms = RoomType.allCases.filter {$0 != RoomType.roomIj}
+
+        ScrollView(.vertical) {


ScrollView([.horizontal, .vertical])
is useful.

…wift Co-authored-by: Ryoya Ito <[email protected]>

… feature/timetable-feature-grid2

charles-b-stb · 2024-08-13T12:18:49Z

@shin-usu Tested with the new TimetableRoom location and there are no problems.

charles-b-stb · 2024-08-13T12:22:09Z

New build fail due to movement of shape?

Error: value of type 'TimetableRoom' has no member 'shape'
timetableItem.room.shape

This built locally but I will investigate the error and fix it before moving forward

shin-usu

I don't confirm all code in this pull request, but I left some comments!
I will check the rest of the code later.

shin-usu · 2024-08-13T12:17:14Z

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

+//
+//  File.swift
+//  
+//
+//  Created by CHARLES BOND on 2024/08/12.
+//


Can you delete file header?

shin-usu · 2024-08-13T12:17:41Z

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

+//
+
+import Foundation
+


Maybe unnecessary line.

Suggested change

shin-usu · 2024-08-13T12:23:15Z

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

+                                if let url = URL(string: speaker.iconUrl) {
+                                    AsyncImage(url: url) {
+                                        $0.image?.resizable()
+                                    }
+                                } else {
+                                    Circle().stroke(Color.gray)
+                                }


Probably, it would be fine following code. Sorry if I am wrong...

AsyncImage(url: URL(string: speaker.iconUrl)) { $0.image?.resizable() } placeholder: { Color.gray }

I copied this code from TImetableCard... maybe they could share this fix?

Yes, I think it can be shared!

The image part was slightly wrong but I fixed it.

… feature/timetable-feature-grid2

charles-b-stb · 2024-08-13T12:27:32Z

Looks like a merge just right after my last pull broke it. I will look for a work-around

ry-itto · 2024-08-13T16:14:21Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

+                        ForEach(rooms, id: \.self) { room in
+
+                            timeBlock.getCellForRoom(room: room, onTap: { item in
+                                store.send(.view(.timetableItemTapped))


TimetableItemWithFavorite instance should be passed to timetableItemTapped 🙏🏼

I need to backmerge for this to work, so I will fix that soon.

shin-usu · 2024-08-13T15:44:48Z

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

+            VStack {
+                //Empty on purpose
+            }


Is it possible to instead it to EmptyView() ?

will try it

Empty view collapses. We need something that will hold the space open.

How about following?

Color.clear .frame(maxWidth: .infinity) .padding(12) .frame(width: 192, height: 153) .clipShape(RoundedRectangle(cornerRadius: 4))

shin-usu · 2024-08-13T15:48:18Z

app-ios/Sources/TimetableFeature/SampleData.swift

Is it correct my understanding that sample data no longer used?
If so, I don't think this file name is appropriate.

The sample data is still used in previews. I can refactor this to break out data class items.

app-ios/Sources/TimetableFeature/TimetableListView.swift

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

shin-usu · 2024-08-13T16:07:17Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

-            EdgeInsets(top: 2,leading: 7, bottom: 2, trailing: 7))
-        .border(highlight ? AssetColors.Custom.flamingo.swiftUIColor : AssetColors.Surface.onSurface.swiftUIColor)
-        .padding(-2)
+func getTimetableRoom(type: RoomType) -> TimetableRoom {


I think it would be more Swift-Like to implement this convert logic in extension.
Also, you can narrow the scope in which this function can be used.

extension RoomType { func toRoom() -> TimetableRoom { ... } } let type: RoomType = .roomI let room = type.toRoom()

shin-usu · 2024-08-13T16:12:12Z

app-ios/Sources/TimetableFeature/TimetableListView.swift

+                jaTitle: "All",
+                enTitle: "All"


According to KMP's implementation, It seems like room IJ's name is "Iguana and Jellyfish"👀

Is there not an item for all rooms? From the UI it looked like some events fill the whole screen.

Bigger question: Do we have any examples of Iguana and Jellyfish? It seems like a case we should test in grid mode.

… feature/timetable-feature-grid2

Co-authored-by: Usuda Shin <[email protected]>

…wift Co-authored-by: Usuda Shin <[email protected]>

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

… feature/timetable-feature-grid2

charles-b-stb · 2024-08-14T15:16:03Z

@shin-usu Responded to comments.

charles-b-stb · 2024-08-15T14:41:58Z

also @ry-itto I responded to your comments.

shin-usu · 2024-08-15T17:12:44Z

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

+                .overlay(RoundedRectangle(cornerRadius: 4).stroke(timetableItem.room.roomTheme.primaryColor, lineWidth: 1))
+            }
+        } else {
+            Spacer()


@charles-b-stb
@ry-itto suggested the Color.clear.
Is there a reason you used Spacer?
I don't think Spacer is often used for this case.

@shin-usu
I didn't see the Color.clear suggestion, sorry.

(This thread is getting very long)

Spacer holds the spot properly so it seemed like a valid use case. It seems more weird to me to use color here but if that is normal for iOS I can switch to that.

@shin-usu @ry-itto Swapped for Color.

I think I have responded to all comments.

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

app-ios/Sources/TimetableFeature/TimetableListView.swift

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

app-ios/Sources/TimetableFeature/TimetableDataItems.swift

shin-usu · 2024-08-17T01:29:33Z

No other places of concern other than ry-itto's comment.
Thanks for the correction😊
Please wait ry-itto's review.

Co-authored-by: Ryoya Ito <[email protected]>

…wift Co-authored-by: Ryoya Ito <[email protected]>

Co-authored-by: Ryoya Ito <[email protected]>

charles-b-stb · 2024-08-17T06:35:37Z

@ry-itto Fixed as recommended. Thank you

MrSmart00

left a comment!
please check it🙏

MrSmart00 · 2024-08-17T23:51:48Z

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift

This view isn't common component I think.
you should move to Timetable package please🙏

I moved it, but it did require me to make one shared class between the card classes public (it was package before)

MrSmart00

THX!
I left a comment!
check it please🙏

MrSmart00 · 2024-08-18T02:59:09Z

app-ios/Sources/TimetableFeature/TimetableGridCard.swift

+import class shared.TimetableItem
+
+public struct TimetableGridCard: View {
+    let timetableItem: TimetableItem?


Could you change this property to be required?
maybe its more than smart and simple i think🤔

@MrSmart00 Currently empty cells depend on this value being able to be null. I don't think I can make this not null-able without breaking empty cells.

Do you have an alternate suggestion for empty cells? 🙇

@charles-b-stb
I know you said situation.
I said means nil check should be out of the view.

you show the view or Color.clear with the property's nil check right?
but Color.clear is not TimetableGridCard view i think🤔

@MrSmart00 I rewrote the code to move Clear outside of TimetableGridCard.

MrSmart00

THX!!!
LGTM👍

charles-b-stb added 6 commits August 12, 2024 16:19

WIP

6be1dfa

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

ab2c5e5

… feature/timetable-feature-grid2

wip

3d84fe1

Grid showing, data not displayable yet.

42a0f8f

Very basic grid in operation. Still needs to fix click operation. Sti…

a8ec52f

…ll need to space welcometalk/lunch/etc.

Code cleanup

398d589

github-actions bot added the type:iOS 🍎 label Aug 12, 2024

charles-b-stb requested review from takahirom and shin-usu August 12, 2024 15:37

ry-itto reviewed Aug 13, 2024

View reviewed changes

charles-b-stb and others added 3 commits August 13, 2024 20:32

Update app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.s…

16190a5

…wift Co-authored-by: Ryoya Ito <[email protected]>

Fixes as per pull request comments.

3ec6e7f

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

7ce2c62

… feature/timetable-feature-grid2

shin-usu reviewed Aug 13, 2024

View reviewed changes

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

3458ce9

… feature/timetable-feature-grid2

Fixed build with suggestions.

0c5fac1

charles-b-stb requested review from shin-usu and ry-itto August 13, 2024 12:47

ry-itto reviewed Aug 13, 2024

View reviewed changes

shin-usu reviewed Aug 13, 2024

View reviewed changes

charles-b-stb and others added 7 commits August 14, 2024 20:34

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

404fec3

… feature/timetable-feature-grid2

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

7ef8e12

Co-authored-by: Usuda Shin <[email protected]>

Update app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.s…

01a633e

…wift Co-authored-by: Usuda Shin <[email protected]>

Update app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.s…

c75137c

…wift Co-authored-by: Usuda Shin <[email protected]>

Update app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.s…

1085d24

…wift Co-authored-by: Usuda Shin <[email protected]>

Fixed various issues.

38eaa49

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

34c3e46

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

charles-b-stb added 3 commits August 14, 2024 21:39

Fixed various issues that came up during pull request.

f201358

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

2a8d3eb

… feature/timetable-feature-grid2

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

0900ab1

shin-usu reviewed Aug 15, 2024

View reviewed changes

charles-b-stb added 2 commits August 16, 2024 21:21

Replaced Spacer with Color as per PR comment

f48e5ba

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

2563d07

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

ry-itto reviewed Aug 16, 2024

View reviewed changes

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

app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.swift Outdated Show resolved Hide resolved

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

charles-b-stb and others added 4 commits August 17, 2024 13:19

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

ab85531

Co-authored-by: Ryoya Ito <[email protected]>

Update app-ios/Sources/CommonComponents/Timetable/TimetableGridCard.s…

bdf3b12

…wift Co-authored-by: Ryoya Ito <[email protected]>

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

348203e

Co-authored-by: Ryoya Ito <[email protected]>

Fixed items broken by pullrequest suggest commits.

b39f2d6

charles-b-stb mentioned this pull request Aug 17, 2024

Feature/446 gridlines #559

Merged

MrSmart00 reviewed Aug 17, 2024

View reviewed changes

Moved GridItem back into the Timetable module.

16262aa

charles-b-stb requested review from ry-itto and MrSmart00 August 18, 2024 01:31

MrSmart00 reviewed Aug 18, 2024

View reviewed changes

charles-b-stb mentioned this pull request Aug 18, 2024

Feature/445 stretchfield #603

Merged

Rewrote to move clear outside TimetableGridCard

45d96ec

MrSmart00 approved these changes Aug 18, 2024

View reviewed changes

charles-b-stb merged commit 20baf7c into main Aug 18, 2024
6 checks passed

charles-b-stb deleted the feature/timetable-feature-grid2 branch August 18, 2024 06:10

		//

		import Foundation

Feature/timetable feature grid2 #367

Feature/timetable feature grid2 #367

Conversation

charles-b-stb commented Aug 12, 2024 • edited Loading

Issue

Overview (Required)

Links

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

Movie (Optional)

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 Aug 13, 2024

charles-b-stb commented Aug 13, 2024 • edited Loading

shin-usu 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 Aug 13, 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

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 Aug 14, 2024

charles-b-stb commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shin-usu commented Aug 17, 2024

charles-b-stb commented Aug 17, 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

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

MrSmart00 left a comment

Choose a reason for hiding this comment

charles-b-stb commented Aug 12, 2024 •

edited

Loading

charles-b-stb commented Aug 13, 2024 •

edited

Loading