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

Adds up navigation in spaces #6073

Merged
merged 8 commits into from
May 30, 2022

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented May 17, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Being in a space or subspace now allows to click an up button on the toolbar or the device back button to navigate up through the spaces

Motivation and context

Closes #3958

I added a new vector icon for the back button instead of using the default inbuilt toolbar one as given we're using a custom toolbar and it very dynamically changes based on space state it's easier to manage it this way. On the topic of using the existing icon, we do have a back icon but the arrow is much longer and stands out as not being the android native icon.

Screenshots / GIFs

backnav.mp4

Tests

  1. Navigate into a space
  2. Click the newly appearing up button on the toolbar
  3. Do the same but instead of the up button use the device's back button
  4. Also try doing this in subspaces. This should work across any number of layers

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 12

Checklist

@ericdecanini ericdecanini added PR-Small PR with less than 20 updated lines and removed PR-Small PR with less than 20 updated lines labels May 17, 2022
@ericdecanini ericdecanini requested review from niquewoodhouse, fedrunov, a team and Florian14 and removed request for a team May 17, 2022 10:19
@github-actions
Copy link

github-actions bot commented May 17, 2022

Unit Test Results

124 files  +  2  124 suites  +2   2m 14s ⏱️ -6s
220 tests +15  220 ✔️ +15  0 💤 ±0  0 ±0 
726 runs  +36  726 ✔️ +36  0 💤 ±0  0 ±0 

Results for commit 90ab67e. ± Comparison against base commit 3674ae7.

This pull request removes 1 and adds 16 tests. Note that renamed tests count towards both.
im.vector.app.features.onboarding.ftueauth.FtueMissingRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given adding an email ThreePid fails with 401, when handling register action, then infer EmailSuccess
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given email verification errors with 401 and succeeds, when checking email validation, then continues to poll until success
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given email verification errors with 401 then fatal error, when checking email validation, then continues to poll until non 401 error
im.vector.app.features.onboarding.ftueauth.MatrixOrgRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email
org.matrix.android.sdk.internal.session.room.aggregation.poll.PollAggregationProcessorTest ‑ given a poll end event for my own poll without enough redaction power level, when processing, then is processed and returns true
org.matrix.android.sdk.internal.session.room.aggregation.poll.PollAggregationProcessorTest ‑ given a poll end event without enough redaction power level, when is processed, then is ignored and return false
org.matrix.android.sdk.internal.session.room.aggregation.poll.PollAggregationProcessorTest ‑ given a poll end event, when processing, then is processed and return true
org.matrix.android.sdk.internal.session.room.aggregation.poll.PollAggregationProcessorTest ‑ given a poll response event after poll is closed, when processing, then is ignored and returns false
org.matrix.android.sdk.internal.session.room.aggregation.poll.PollAggregationProcessorTest ‑ given a poll response event which is already processed, when processing, then is ignored and returns false
org.matrix.android.sdk.internal.session.room.aggregation.poll.PollAggregationProcessorTest ‑ given a poll response event which is not one of the options, when processing, then is ignored and returns false
…

♻️ This comment has been updated with latest results.

Copy link
Contributor

@Florian14 Florian14 left a comment

Choose a reason for hiding this comment

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

Code seems good to me, my first impression was that the new navigation seemed a bit weird to me, I didn't realize you were going up into the hierarchy of the spaces by opening the drawer. But in real usage, we would see more rooms in the rooms list by navigating up, so I guess it makes sense

@ericdecanini
Copy link
Contributor Author

Failed checkk seems unrelated

Copy link
Contributor

@fedrunov fedrunov left a comment

Choose a reason for hiding this comment

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

LGTM

@ericdecanini ericdecanini merged commit b8c0c61 into develop May 30, 2022
@ericdecanini ericdecanini deleted the feature/eric/improve-back-navigation branch May 30, 2022 08:38
@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    = passed=20 failures=0 errors=0 skipped=3
  • [org.matrix.android.sdk.account]
    = passed=3 failures=0 errors=0 skipped=2
  • [org.matrix.android.sdk.internal]
    = passed=27 failures=1 errors=0 skipped=1
  • [org.matrix.android.sdk.ordering]
    = passed=16 failures=0 errors=0 skipped=0
  • [org.matrix.android.sdk.PermalinkParserTest]
    = passed=2 failures=0 errors=0 skipped=0

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.

Improve interaction for switching spaces
4 participants