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

UI Updates to Document Data Screen #797

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dritan-x
Copy link
Contributor

  • Added warning message text, box, and icon
  • Showing document data fields according to a specified order defined in the code that matches the slides. Shows remaining fields in an unordered fashion.

Tested by:

  • Manual testing by adding Test eID-Karte Personalausweis
  • ./gradlew check
  • ./gradlew connectedCheck

Screenshot 2024-11-22 at 6 21 20 PM

Screenshot 2024-11-22 at 6 21 46 PM

- Added warning message text, box, and icon
- Showing document data fields according to a specified order defined in the code that matches the slides. Shows remaining fields in an unordered fashion.

Tested by:
- Manual testing by adding Test eID-Karte Personalausweis
- ./gradlew check
- ./gradlew connectedCheck

Signed-off-by: dritan-x <[email protected]>
@dritan-x dritan-x requested a review from kdeus November 23, 2024 02:27
Copy link
Contributor

@kdeus kdeus left a comment

Choose a reason for hiding this comment

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

Thanks for this. I don't have any major concerns, just several small little details.

)

val sortedKeys = centerAttributes.keys.sortedBy {keyValue->
if (keyValue in orderedEntries){
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Missing a space after the parenthesis.

tint = MaterialTheme.colorScheme.background.inverse(),
contentDescription = stringResource(
R.string.accessibility_artwork_for,
stringResource(id = R.string.document_details_screen_title)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The warning icon isn't really artwork for the document screen. I don't see any other accessibility text that's a better match... Maybe we need a new string for accessibility, "Warning icon," or something like that?

)
)
Text(
modifier = Modifier.padding(top = 20.dp, bottom = 20.dp, end=5.dp),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The end padding looks a little small. From the screenshot, it feels a little tight on the right side, compared to everywhere else. Maybe 16.dp or 20.dp, so it's closer to the top/bottom?

"family_name",
)

val sortedKeys = centerAttributes.keys.sortedBy {keyValue->
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we'd normally put spaces around keyValue here?

) {
Row(
modifier = Modifier
.padding(horizontal = 30.dp)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It looks like all the elements on the screen should be indented from the left so they line up with this box. Maybe move this from this Row to the Column that contiains everything?

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.

2 participants