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

ANDROAPP-5496-mobile-ui-Input-With-Icon-Cards #65

Merged

Conversation

msasikanth
Copy link
Collaborator

No description provided.

@msasikanth msasikanth force-pushed the ANDROAPP-5496-mobile-ui-Input-With-Icon-Cards branch from 30fd699 to fb34c8c Compare September 14, 2023 10:08
@msasikanth
Copy link
Collaborator Author

Thought about two approaches, one is using scoped composables similar to LazyColumn or LazyRow. But we would essentially expose a way to create custom cards as well. Since that's something we don't want, went with approach that takes in data to render the cards internally.

@msasikanth msasikanth requested a review from andresmr September 14, 2023 12:27
import org.hisp.dhis.mobile.ui.designsystem.theme.Spacing

@Composable
internal fun VerticalGrid(
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently we have a file called Container that has a few other container type components we are using, maybe we could refactor this class into it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just an internal component that we are not exposing publicly, that's why I placed it in the internal package.

supportingText: List<SupportingTextData>? = null,
legendData: LegendData? = null,
isRequired: Boolean = false,
testTag: String = "",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting, I was thinking that we don't need to tag it for the test if you are already tagging it below, but I hadn't thought about the user and if they have any way of adding their own test tags through that param.
we should study whether to add it to the rest of the components or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I felt like it would be useful for testing certain elements in a list of items. But I am fine with removing them from children composables. Something I wanted to try that's all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea 👍

@msasikanth msasikanth force-pushed the ANDROAPP-5496-mobile-ui-Input-With-Icon-Cards branch 2 times, most recently from 62dd683 to 3f7c9e5 Compare September 15, 2023 09:01
@msasikanth msasikanth force-pushed the ANDROAPP-5496-mobile-ui-Input-With-Icon-Cards branch from 3f7c9e5 to 6c342d7 Compare September 15, 2023 09:08
@msasikanth msasikanth requested a review from andresmr September 15, 2023 09:48
@msasikanth msasikanth force-pushed the ANDROAPP-5496-mobile-ui-Input-With-Icon-Cards branch 3 times, most recently from 7680662 to 53095e2 Compare September 19, 2023 03:54
@msasikanth msasikanth force-pushed the ANDROAPP-5496-mobile-ui-Input-With-Icon-Cards branch 2 times, most recently from 5bf8cf5 to e00c2d3 Compare September 26, 2023 10:16
It is affecting layout height and nested scrolling on the input shell components
We don't need horizontal grid at this point, but we have to invert the column and row usages and create a new component.
Since we removed this from the `InputShell` we are applying it in `BasicTextInput` directly since this is the only component currently we need this for.
I had to extract out the `IconCard` into separate internal file. For now I moved the `IconCardData` in that file as well. But we can extract out models into separate file later on.
One of the issues with existing bottom/button shadows extension function is, it doesn't change the change of the actual component. But we want to modify the size so that we don't have to offset the component it self.

Why we want that? In situation like grid or list or any component where the component gets clipped, the offset will get clipped as well. That would mean the component it self will get clipped. We don't want that. We can work around that by telling the component to avoid clipping or placing padding etc, but we would have to do that for all components that use it.

So, added a iconCardShadow modifier that would maintain the size and add shadow radius bottom padding to increase the size instead.
@msasikanth msasikanth force-pushed the ANDROAPP-5496-mobile-ui-Input-With-Icon-Cards branch from e00c2d3 to 31d2c37 Compare September 28, 2023 03:24
@msasikanth msasikanth merged commit c80171b into dhis2:main Sep 28, 2023
2 checks passed
@msasikanth msasikanth deleted the ANDROAPP-5496-mobile-ui-Input-With-Icon-Cards branch September 28, 2023 04:07
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.

3 participants