Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #17800: Request desktop site from home screen. #18653

Merged
merged 1 commit into from
Mar 30, 2021
Merged

For #17800: Request desktop site from home screen. #18653

merged 1 commit into from
Mar 30, 2021

Conversation

mcarare
Copy link
Contributor

@mcarare mcarare commented Mar 26, 2021 •
edited by eliserichards

For #17800

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@mcarare mcarare added the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Mar 26, 2021
@mcarare mcarare force-pushed the 17800 branch 9 times, most recently from 0e56809 to 8b096a7 Compare March 29, 2021 14:40
@mcarare mcarare added needs:review PRs that need to be reviewed and removed pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on labels Mar 29, 2021
@mcarare mcarare marked this pull request as ready for review March 29, 2021 16:24
@mcarare mcarare requested review from a team as code owners March 29, 2021 16:24
eliserichards
eliserichards suggested changes Mar 29, 2021
View reviewed changes
app/src/main/java/org/mozilla/fenix/HomeActivity.kt Outdated
@@ -808,7 +810,8 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
newTab: Boolean,
engine: SearchEngine?,
forceSearch: Boolean,
flags: EngineSession.LoadUrlFlags = EngineSession.LoadUrlFlags.none()
flags: EngineSession.LoadUrlFlags = EngineSession.LoadUrlFlags.none(),
requestDesktopMode: Boolean? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should default to false too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

app/src/main/java/org/mozilla/fenix/HomeActivity.kt Outdated
ContentAction.UpdateDesktopModeAction(
components.core.store.state.selectedTabId.toString(), true
))
settings().openNextTabInDesktopMode = false
Copy link
Contributor

Choose a reason for hiding this comment

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

is this resetting the mode? it's a bit confusing having this within the if (requestDesktopMode == true) block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I added a short comment.

app/src/main/java/org/mozilla/fenix/HomeActivity.kt Outdated
Comment on lines 833 to 840
val requestDesktopSiteUseCase =
components.useCases.sessionUseCases.requestDesktopSite
requestDesktopSiteUseCase.invoke(requestDesktopMode)
components.core.store.dispatch(
ContentAction.UpdateDesktopModeAction(
components.core.store.state.selectedTabId.toString(), true
))
settings().openNextTabInDesktopMode = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: create a helper function here (handleRequestDesktopMode())

@mcarare mcarare changed the title Request desktop site from home screen. For #17800: Request desktop site from home screen. Mar 30, 2021
gabrielluong
gabrielluong approved these changes Mar 30, 2021
View reviewed changes
app/src/main/java/org/mozilla/fenix/home/HomeMenu.kt
@@ -193,6 +203,9 @@ class HomeMenu(
syncedTabsItem,
bookmarksItem,
historyItem,
BrowserMenuDivider(),
desktopItem,
Copy link
Member

Choose a reason for hiding this comment

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

I am curious what designs we are using since the home menu ordering doesn't look anything like this anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am just inserting this item here, I am somewhat unsure on the final designs and if there is any open issue about reordering menu items in the home screen,

Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the reordering is in #17770

app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt Show resolved Hide resolved
app/src/main/java/org/mozilla/fenix/search/toolbar/ToolbarView.kt Show resolved Hide resolved
app/src/main/java/org/mozilla/fenix/utils/Settings.kt
@@ -1020,4 +1020,9 @@ class Settings(private val appContext: Context) : PreferencesHolder {
default = false,
featureFlag = FeatureFlags.addressesFeature
)

var openNextTabInDesktopMode by booleanPreference(
Copy link
Member

Choose a reason for hiding this comment

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

Our list of settings is getting a bit long. Would you mind adding a comment to describe this preference for future documentation purposes?

@gabrielluong gabrielluong added pr:approved PR that has been approved and removed needs:review PRs that need to be reviewed labels Mar 30, 2021
@gabrielluong gabrielluong self-assigned this Mar 30, 2021
eliserichards
eliserichards approved these changes Mar 30, 2021
View reviewed changes
Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

@gabrielluong gabrielluong merged commit ccfb275 into mozilla-mobile:master Mar 30, 2021
@cadeyrn cadeyrn mentioned this pull request Mar 31, 2021
Closed
@eliserichards eliserichards linked an issue Apr 5, 2021 that may be closed by this pull request
[Feature] Request desktop site from new tab menu #17800
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gabrielluong gabrielluong gabrielluong approved these changes

@eliserichards eliserichards eliserichards approved these changes

Assignees

@gabrielluong gabrielluong

Labels
pr:approved PR that has been approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Request desktop site from new tab menu
3 participants
@mcarare @gabrielluong @eliserichards