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

Remove the extra gap during onboarding #5141

Merged

Conversation

0nko
Copy link
Member

@0nko 0nko commented Oct 15, 2024

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208542610603557/f

Description

This PR removes the extra gap that was visible during onboarding. The issue was mainly caused by this. Removing it, however, had a cascading side effect:

  • A behavior had to be added to the new browser tab layout so that the NTP content would stay below the address bar now that the padding was removed
  • The change above, caused some new layout changes, for example the button on the NTP was now out of the viewport, so a bottom margin had to be added
  • The change above had a side effect that an unwanted gap was added to the browser view during the partial onboarding, so some logic had to be added bot to the bottom and top bar behavior

Steps to test this PR

The toolbar is at the top

  • Test the entire onboarding flow with the bar at the top and verify no unwanted gaps are present
  • Test that the NTP looks correct, with the Edit button visible at the bottom
  • Test the autocomplete list is scrollable and all items are visible
  • Test that browser layout looks correct with no unwanted gaps

The toolbar is at the bottom

  • Test the entire onboarding flow with the bar at the top and verify no unwanted gaps are present
  • Test that the NTP looks correct, with the Edit button visible at the bottom
  • Test the autocomplete list is scrollable and all items are visible
  • Test that browser layout looks correct with no unwanted gaps

nalcalag and others added 29 commits October 14, 2024 12:29
Task/Issue URL: https://app.asana.com/0/1207908166761516/1208267346828963/f 

### Description

Adds a new DaxBubbleCardViewExperiment for upcoming experiment with onboarding.

These are not visible will be used later in another task. To see these dialogs they will need to be enabled in code.

### Steps to test this PR

_Launch onboarding_
- [ ] Ensure dialog shape and arrows have not changed 

### UI changes

There should be none. But here are examples of what the dialogs should look like when shown:

![Screenshot_20240926_121050](https://github.com/user-attachments/assets/480f2f4e-9e8d-44c7-a48c-48017f1afc9b)

![Screenshot_20240926_122205](https://github.com/user-attachments/assets/219a0875-9874-471e-ac01-bc83a8a4cc2f)

![Screenshot_20240926_155207](https://github.com/user-attachments/assets/17f96275-896a-4850-9ffe-96bb9716a8d9)
@0nko 0nko requested a review from nalcalag October 15, 2024 23:11
@nalcalag nalcalag self-assigned this Oct 19, 2024
@nalcalag nalcalag force-pushed the feature/noelia/highlights_pre_onboarding branch from 404a508 to 0355839 Compare October 21, 2024 12:22
…ej/onboarding_gap

# Conflicts:
#	app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
#	app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
#	app/src/main/java/com/duckduckgo/app/browser/omnibar/TopAppBarBehavior.kt
#	app/src/main/res/layout/include_new_browser_tab.xml
Copy link
Contributor

@nalcalag nalcalag left a comment

Choose a reason for hiding this comment

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

@0nko when I set the address bar to the bottom, the new tab page becomes blank.

Video attached:

Screen_recording_20241021_155111.webm

Copy link
Contributor

@nalcalag nalcalag left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing everything 💯

@0nko 0nko merged commit ad24b48 into feature/noelia/highlights_pre_onboarding Oct 22, 2024
5 checks passed
@0nko 0nko deleted the fix/ondrej/onboarding_gap branch October 22, 2024 13:52
nalcalag added a commit that referenced this pull request Oct 22, 2024
Task/Issue URL:
https://app.asana.com/0/1207418217763355/1208542610603557/f

### Description

This PR removes the extra gap that was visible during onboarding. The
issue was mainly caused by
[this](https://github.com/duckduckgo/Android/compare/feature/noelia/highlights_pre_onboarding...fix/ondrej/onboarding_gap?expand=1#diff-38c52e4c3d2d7891eed9656823f30c41a4984cdeaa5f89a78e31f57340f6afbeL39).
Removing it, however, had a cascading side effect:

- A
[behavior](https://github.com/duckduckgo/Android/compare/feature/noelia/highlights_pre_onboarding...fix/ondrej/onboarding_gap?expand=1#diff-38c52e4c3d2d7891eed9656823f30c41a4984cdeaa5f89a78e31f57340f6afbeR25)
had to be added to the new browser tab layout so that the NTP content
would stay below the address bar now that the padding was removed
- The change above, caused some new layout changes, for example the
button on the NTP was now out of the viewport, so a bottom margin [had
to be
added](https://github.com/duckduckgo/Android/compare/feature/noelia/highlights_pre_onboarding...fix/ondrej/onboarding_gap?expand=1#diff-38c52e4c3d2d7891eed9656823f30c41a4984cdeaa5f89a78e31f57340f6afbeR45)
- The change above had a side effect that an unwanted gap was added to
the browser view during the partial onboarding, so some logic had to be
added bot to the
[bottom](https://github.com/duckduckgo/Android/compare/feature/noelia/highlights_pre_onboarding...fix/ondrej/onboarding_gap?expand=1#diff-fe6ebde9b11eddd212b40d9b12701a7e23e083cfc919c06b7a084d8f3e61154dR60)
and
[top](https://github.com/duckduckgo/Android/compare/feature/noelia/highlights_pre_onboarding...fix/ondrej/onboarding_gap?expand=1#diff-f5d61424e875a30ac1059375679a3bebb00e754e93a33f01d026019618bea1c0R45-R48)
bar behavior

### Steps to test this PR

_The toolbar is at the **top**_

- [x] Test the entire onboarding flow with the bar at the top and verify
no unwanted gaps are present
- [x] Test that the NTP looks correct, with the Edit button visible at
the bottom
- [x] Test the autocomplete list is scrollable and all items are visible
- [x] Test that browser layout looks correct with no unwanted gaps

_The toolbar is at the **bottom**_

- [x] Test the entire onboarding flow with the bar at the top and verify
no unwanted gaps are present
- [x] Test that the NTP looks correct, with the Edit button visible at
the bottom
- [x] Test the autocomplete list is scrollable and all items are visible
- [x] Test that browser layout looks correct with no unwanted gaps

---------

Co-authored-by: Noelia Alcala <[email protected]>
Co-authored-by: Mike Scamell <[email protected]>
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