-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat/event overview UI #35
Conversation
This screen allows you to overview any event you have selected with all relevant information (no. of participants, date, location, etc...). A minimap also diplays the location of the park where the event is hosted
tests will check if components are correctly displayed as well as dasboard interactions note: some older tests have been removed from the bottomNavigation tests as well as the MapUI tests due to new functionalities from the main not working well with them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good! Left a comment for the tests, I think you forgot to implement one as fullevent
var is never used.
However there are a few things that I would change about the UI:
- For the top app bar where the event name is, I would center it and not have the text underlined, I think it would go better with the rest of the ui's app. I would also set the background color of the bar to
MaterialTheme.colorScheme.surface
to match the same style of the other screen's top app bar (as it is on figma) - This feels empty (see picture below), I would put two of them on the same row, one below and center them to fill the space better
- I would also center the text in the details section to be more inlined with the existing UI in the app
- Put a background color behind the Details text so that we can easily see where it ends
- Change the bottom button text from 'Join an event' to 'Join this event' to remove ambiguity
app/src/androidTest/java/com/android/streetworkapp/ui/park/EventOverviewTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My review
Very good code, I will split the parts of EventOverviewScreen
in order to follow the coaches' recommendations.
My opinion of the review @Alex720p
- For the top app bar where the event name is, I would center it and not have the text underlined, I think it would go better with the rest of the ui's app. I would also set the background color of the bar to
MaterialTheme.colorScheme.surface
to match the same style of the other screen's top app bar (as it is on figma)
The concept of using the same conventions for all parts of a project is called principles of atomic design.
- This feels empty (see picture below), I would put two of them on the same row, one below and center them to fill the space better
Since I haven't seen the design on Figma, is it an empty space left unintentionally? If not, it needs to be filled in.
- I would also center the text in the details section to be more inlined with the existing UI in the app
Center les object is a way to fill the space. I'll add an image OR center it, but not both.
- Put a background color behind the Details text so that we can easily see where it ends
It's a possibility to see if it really increases visibility, which is probably where I'm least convinced.
- Change the bottom button text from 'Join an event' to 'Join this event' to remove ambiguity
Yes, it clarifies the effect of the button.
app/src/androidTest/java/com/android/streetworkapp/ui/park/EventOverviewTest.kt
Outdated
Show resolved
Hide resolved
This not intentional but the figma design is actually pretty different from the current product as the figma was a very simple skeleton of what the screen could be Suggested changes feel very relevant though and will be applied
I agree that centering feels a little too "artificial", I can try adding an placeholder image for now that could be changed for each event
Since the description is possibly scrollable if too long to fit in the screen i do think it might help to increase the tab's visibility to delimit where it ends, i'll experiment a bit to see what seems to fit best Thank you to @Alex720p and @tercierp for the insightfull reviews! |
…on pr review park image has been added on empty spot, minor typos were corrected and a background color has been added on dashboard for visibility
…rameter location from testPark on Mainactivity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are very good but you should check my question and my suggestion
app/src/main/java/com/android/streetworkapp/ui/park/EventOverview.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/android/streetworkapp/ui/park/EventOverview.kt
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
Overview
This branch introduces the event overview screen, which will allow users to see a detailed overview of an ongoing event.
This screen will be accessed via either the park overview screen or the event timeline which will come later on in the project
features
The Screen offers a variety of feature such as:
Testing
You can preview the screen by simply uncommenting the preview written at the end of
EventOverview.kt
class (note: as this preview is not done on the main, navigation cannot be tested, but since the only navigating feature of this screen calls thegoBack
function, i think it is fine to ignore it for now)Additional notes