create location screen #44
Conversation
| import org.example.project.domain.entity.District | ||
| import org.example.project.domain.entity.Governorates | ||
| import org.example.project.domain.repository.LocationRepository | ||
| import org.koin.core.annotation.Provided |
There was a problem hiding this comment.
Remove redundant imports
|
can you attach a screenshot of the created screen? |
composeApp/src/commonMain/kotlin/org/example/project/data/dto/DistrictDto.kt
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/example/project/domain/entity/District.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
the file name is Location, but it has GovernorateSelector and DetailLocationInput, I don't get the idea of the file, if you use them as reusable components for the location screen, you can have them in 2 different files, of have a meaningful file name
...oseApp/src/commonMain/kotlin/org/example/project/presentation/components/DropdownSelector.kt
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/example/project/presentation/components/Location.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/example/project/presentation/components/Location.kt
Outdated
Show resolved
Hide resolved
| BackButton() | ||
| ProgressIndicator( | ||
| currentPage = currentPage, | ||
| totalPage = 4, |
There was a problem hiding this comment.
the page count should not be hard coded, pass it plz
There was a problem hiding this comment.
it will delete it when integration using scaffold
| { | ||
| Icon( | ||
| painter = painterResource(Res.drawable.arrow_left), | ||
| contentDescription = "back button", |
There was a problem hiding this comment.
plz avoid hard coded contentDescription
| val displayText by remember(state.selectedGovernorate, state.selectedDistrict) { | ||
| derivedStateOf { | ||
| val parts = listOf(state.selectedGovernorate, state.selectedDistrict).filter { it.isNotBlank() } | ||
| if (parts.isEmpty()) "Governorate, District" else parts.joinToString(", ") |
There was a problem hiding this comment.
don't you believe it's logic can be moved to the viewmodel? also avoid hard coded strings plz
| } | ||
| } | ||
|
|
||
| Column( |
There was a problem hiding this comment.
why don't we have the content itself in a separated content function? follow state hoisting plz
| ) { | ||
| AccountSetupTopBar( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| currentPage = 2 |
There was a problem hiding this comment.
we need to handle the logic of the topbar in a better way, to avoid the magical numbers
|
No description provided.