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

belindas-closet-android_2_97_navigation-bar #114

Merged
merged 9 commits into from
Nov 8, 2023

Conversation

Annelisebx
Copy link
Contributor

@Annelisebx Annelisebx commented Nov 1, 2023

Resolves Issue 97

As a developer, I did the following:

  • create a top navigation bar displaying tabs that separate the products by product type
  • each tab is a 'category' of items
  • the tab should be visible from all product screens (in-progress)

image

We also want to include unit tests if and when possible to make sure we're getting full coverage of our code. This will simulate a real world environment where tests are extremely important.

Unit Test Example

@Annelisebx Annelisebx self-assigned this Nov 1, 2023
@Annelisebx Annelisebx changed the title TopAppBar Navigation by Category belindas-closet-android_2_97_navigation-bar Nov 1, 2023
@Annelisebx Annelisebx added the enhancement New feature or request label Nov 1, 2023
This was linked to issues Nov 1, 2023
@taylorpapke
Copy link
Contributor

@Annelisebx I've updated and added a comment to user story #74 as I realized it needed some more context. Yes, what you have implemented is the right idea but I do have a couple comments:

  1. After seeing how the tabs look and appear to function, I think we should move to a dropdown that lists the categories since there will be 15 or so categories.
  2. The dropdown can come in a later PR. For now, I think we should be concerned with just creating a navigation bar with the back button, whatever other button (edit, etc.) the page has and we could simple have a button labeled "products" that can redirect to the home page with all categories until we can set it up as a dropdown with the different categories routing to those specific product categories.
    I also think you should reach out to @kkennedy-gh because he is essentially working on the same thing and has come up with a good strategy for completing this rather daunting task.

@taylorpapke taylorpapke self-requested a review November 1, 2023 18:26
Copy link
Contributor

@taylorpapke taylorpapke left a comment

Choose a reason for hiding this comment

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

This is the right idea but I'd like to have the back and edit buttons still functional and in their proper place (not overlapping). I think we should move from the tabs to a dropdown menu but we can simply just have a "products" button for the sake of this PR and have it redirect to the home page.
Screenshot 2023-11-01 at 11 38 11 AM

@Annelisebx
Copy link
Contributor Author

@taylorpapke Hi Taylor,

  1. I understand what you are saying about drop down/drawers opposed to tabs. I have removed those.
  2. The navigation bar function and layout makes sense. One question: Which screens does this need to be on? It is currently on the IndividualProduct page. The back arrow should go back to the the 'Home' screen? This is currently the only screen that shows a list of products, correct?

Thank you for all this input.

@taylorpapke
Copy link
Contributor

taylorpapke commented Nov 2, 2023

@taylorpapke Hi Taylor,

  1. I understand what you are saying about drop down/drawers opposed to tabs. I have removed those.
  2. The navigation bar function and layout makes sense. One question: Which screens does this need to be on? It is currently on the IndividualProduct page. The back arrow should go back to the the 'Home' screen? This is currently the only screen that shows a list of products, correct?

Thank you for all this input.

@Annelisebx

  • This navigation bar should be on each page. @kkennedy-gh has come up with a good strategy on implementing this on nsc-events so I would recommend looking at nsc-events-android and reaching out to him about how he is going about placing the nav bar on each page in separate issues/PR's.
  • The back arrow should always route back to the page that you came from so individual product page should go back to the product page. Product page should go back to the home page. Home page shows a list of product categories. Product page shows a list of products from a product category.

@tinpham5614
Copy link
Contributor

I just want to add a YouTube tutorial as a reference for Scaffold, Top App Bar, ArrowBack, etc.
You can modify other actions to suit the requirements
https://www.youtube.com/watch?v=EqCvUETekjk&t=352s

@Annelisebx
Copy link
Contributor Author

This is my understanding of the flow and what I am trying to implement through a TopAppBar.
image

Updated:

  • added 'Menu' icon to ProductDetails

Next:

  • Get menu to open and list product categories

image

@taylorpapke
Copy link
Contributor

Hi @Annelisebx,
I like the diagram you provided and what you have done with the nav bar. I now realize that the scope of this user story is quite large given the fact that it affects every pages structure. I’ve decided to narrow the scope for this user story (see the updated version and comment here. Just do the same thing that you have done here for the rest of the pages and don’t worry about any functionality for the product menu icon. I will need to get more info from the client on how they want this to function. This will allow me a visual aid to try to gather more requirements from them. I will create a new user story in our next sprint to deal with functionality.

Let's just fix the merge conflicts and this should be good for this screen.

@taylorpapke
Copy link
Contributor

taylorpapke
taylorpapke previously approved these changes Nov 5, 2023
@taylorpapke taylorpapke closed this Nov 5, 2023
@taylorpapke taylorpapke reopened this Nov 5, 2023
Robel-003
Robel-003 previously approved these changes Nov 7, 2023
Copy link
Contributor

@Robel-003 Robel-003 left a comment

Choose a reason for hiding this comment

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

Just need to resolve some conflicts in the main file and you should be good.

@Annelisebx Annelisebx dismissed stale reviews from Robel-003 and taylorpapke via b10e7ea November 7, 2023 19:15
@Annelisebx
Copy link
Contributor Author

Resolved conflicts in Main on Home.kt file

@brinkbrink
Copy link
Contributor

Let us know if you need any support with the merge conflict! GitHub docs all about merge conflicts.

…to right side of Nav bar and edit icon. No functionality.
…n-01' into enhancement-97-product-navigation-01
@Annelisebx
Copy link
Contributor Author

Annelisebx commented Nov 7, 2023

@brinkbrink @taylorpapke @Robel-003
Merge conflicts resolved. Ready for review.

  • Menu icon added to both ProductDetail.kt and IndividualProduct.kt.
  • Icon is placed to right of edit button and not overlapping.
  • No functionality yet. All other pages/screens remain the same.

image

Copy link

@TuongPhamM TuongPhamM left a comment

Choose a reason for hiding this comment

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

I see you added Menu button which changes drawerStates. Should it ideally be a one-time use button since it only changes state to Open?

@taylorpapke taylorpapke self-requested a review November 8, 2023 02:35
@Annelisebx
Copy link
Contributor Author

I see you added Menu button which changes drawerStates. Should it ideally be a one-time use button since it only changes state to Open?
@TroyFPV48
I think it would ideally change back to closed when clicked out of it. However, this button has no functionality right now. We are going to create a new issue/user story for adding content once confirmed with client what is wanted in the menu.

@taylorpapke
Copy link
Contributor

taylorpapke commented Nov 8, 2023

@Annelisebx It looks great on the pages where there is a nav bar but there are images overlapping on the home page and the back button has disappeared from all other pages:
Screenshot 2023-11-07 at 10 32 20 PM
Screenshot 2023-11-07 at 10 32 41 PM
Screenshot 2023-11-07 at 10 32 59 PM
I think we should try to retain the integrity and functionality of the app in its best state as we move forward, even if we intend to replace those areas with something else in future PR's.

Copy link
Contributor

@taylorpapke taylorpapke left a comment

Choose a reason for hiding this comment

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

  1. change home page back to what it currently is in main.
  2. change the pages without nav bar to include back buttons where they currently are in main.

perhaps go back to a prior commit or start a new pr from main and copy/paste the changes you want?

@brinkbrink brinkbrink merged commit 954a3d8 into main Nov 8, 2023
1 check passed
@brinkbrink brinkbrink deleted the enhancement-97-product-navigation-01 branch November 8, 2023 17:36
@Annelisebx
Copy link
Contributor Author

@brinkbrink Was this merged already? I don't think we wanted to merge it because I accidentally changed the Home page (and others unintentionally)

If it has been merged and changed, I will open a new issue/PR for fixing back to what @taylorpapke commented above:

  • Restore Home page
  • Add back arrow to all other screens

@Annelisebx Annelisebx mentioned this pull request Nov 8, 2023
@brinkbrink brinkbrink restored the enhancement-97-product-navigation-01 branch November 8, 2023 19:27
brinkbrink added a commit that referenced this pull request Nov 8, 2023
brinkbrink added a commit that referenced this pull request Nov 8, 2023
@brinkbrink
Copy link
Contributor

@brinkbrink Was this merged already? I don't think we wanted to merge it because I accidentally changed the Home page (and others unintentionally)

If it has been merged and changed, I will open a new issue/PR for fixing back to what @taylorpapke commented above:

  • Restore Home page
  • Add back arrow to all other screens

Hello @Annelisebx I reverted this and it is now open here at #136. I saw two approving reviews so I merged. Didn't read the comments because I noticed your request on Slack:

"Can I please get 2 reviews on my PR 114 so I can merge? I went over issue and branch today with BC in class and he said to merge and complete and finish remaining on new issue/branch, thank you!"

@Annelisebx
Copy link
Contributor Author

@brinkbrink thank you!! Sorry, that was last night before the reviews which caught the error. I will try to fix ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Create Navigation Bar USER STORY: Navigation Bar
6 participants