Page MenuHomePhabricator

Unify the logic to brand the search functionality across mobile modes
Closed, ResolvedPublic2 Story Points

Description

We currently brand the search field differently when in beta - but this logic could easily be done inside MinervaTemplate.php. This will be useful when we remove SkinMinervaBeta.

A/C

  • Make sure search bar placeholder text branding logic happens in SkinMinerva.

Event Timeline

Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptOct 14 2016, 4:19 PM
bmansurov moved this task from Uncategorized to Clean-Up on the Technical-Debt (RW-Tech-Debt) board.
ovasileva set the point value for this task to 2.Oct 19 2016, 4:50 PM
ovasileva added a subscriber: ovasileva.

@bmansurov to update description

@Nirzar how does this tie into the work I've been doing for you in the new header prototypes?

description isn't sufficient. is it about (beta) written in search place holder.
i have a card that moves the (beta) from there to the navigation drawer.

@Jdlrobson i think it's a great idea to change the wordmark if your in beta. maybe add our beta logo to the wordmark :)

I think it was about the placeholder when I created this task from @Jdlrobson's description of the parent task.

@Nirzar - yeh we used to do that when we previously had branding which is why I feel you could create a task for the new header design which would allow us to use the same placeholder in both stable and beta,

bmansurov updated the task description. (Show Details)Oct 19 2016, 7:16 PM

@Jdlrobson let's do that...

bmansurov moved this task from To Do to Doing on the Reading-Web-Sprint-84-Zero-minutes-left board.

Change 317572 had a related patch set uploaded (by Bmansurov):
Unify the logic to brand the search functionality across mobile modes

https://gerrit.wikimedia.org/r/317572

Both @Florian and @Jdlrobson have left comments. I'm not sure that my original comment was responded to but it goes to what I said in T147944#2717328.

@phuedx, can you clarify what you meant by your original comment? Rather, tell me how you'd get rid of the if statement yourself without extending the skin. Thanks.

phuedx added a comment.EditedOct 27 2016, 2:44 PM

@phuedx, can you clarify what you meant by your original comment? Rather, tell me how you'd get rid of the if statement yourself without extending the skin. Thanks.

Having re-read my original comment I admit that I put too much emphasis on the if statement. I don't think that I said or implied that I could get rid of the if statement either – indeed, that's what the overriding method does!

SkinMinervaBeta overrides the behaviour of SkinMinerva in certain circumstances. Hopefully, we house the beta-specific logic in the former, thereby keeping the latter isolated. I don't see the value in moving beta-specific logic into SkinMinerva.

Another concern is that the

if ( $this->getMode() === 'beta' ) {
  // Do one thing.
} else {
  // Do the default thing.
}

stanza will be repeated if we continue removing overriding methods in the name of removing SkinMinervaBeta.

Is my concern premature?

This is a unique situation where we need to make it clear to users that they are in beta mode (i.e. all experimental features are enabled). For this reason I think it's OK to have an if statement. As for other features, we can keep using flags.

My only remaining worry is not with the if statement but the way we change the value of getMode.

Currently SkinMinervaBeta extends SkinMinerva and overrides it. We should also be very careful not to invoke MobileContext inside a desktop environment even if it is on the Minerva skin. I think we talked about the Skin taking some options on initialisation including a turn on beta config flag but it's not completely 100% clear to be if that's the approach we are running with.

Change 317572 merged by jenkins-bot:
Unify the logic to brand the search functionality across mobile modes

https://gerrit.wikimedia.org/r/317572

bmansurov removed bmansurov as the assignee of this task.Oct 31 2016, 7:50 PM

@Jhernandez @jhobs @phuedx can one of you please sign off?

We should also be very careful not to invoke MobileContext inside a desktop environment even if it is on the Minerva skin.

Out of curiosity, why? Is there a technical reason apart from simplifying SkinMinerva?

Anyway, there appear to be a handful things that SkinMinerva gets from MobileContext:

  1. Whether the user is a member of the beta group (or "MobileFrontend is in beta mode" as we like to say) – seriously, we should align our language with that of the codebase…
  2. Whether the mobile view should be displayed (see MobileContext#shouldDisplayMobileView).
  3. Access to the global configuration, i.e. an instance of GlobalVarConfig with the default 'wg' prefix.
  4. Access to something capable of generating URLs for the mobile domain (see MobileContext#getMobileUrl).

1, 2, and 3 are just state. 4 should be extracted into its own class, which would hide away the complexity of generating URLs and for which domains. All of them, however, could be injected:

includes/SkinMinerva.php
public function __construct( $shouldDisplayMobileView, $isBetaGroupMember, Config $config, Router $router ) {
  $this->isMobileMode = $shouldDisplayMobileView;
  $this->isBetaGroupMember = $isBetaGroupMember;
  $this->config = $config;
  $this->router = $router;

  // ...
}

With the above, the various instances of $this->mobileContext... could be removed.

@phuedx would you create a separate task to capture your thoughts in T148197#2759519? Thanks.

phuedx claimed this task.Nov 1 2016, 3:43 PM

We should also be very careful not to invoke MobileContext inside a desktop environment even if it is on the Minerva skin.

Out of curiosity, why? Is there a technical reason apart from simplifying SkinMinerva?

  1. On the long term SkinMinerva needs to be in its own repo. This has been discussed to death, but skins don't live inside extensions and things like MobileContext do not belong in skins. Thus using MobileContext inside SkinMinerva presents a problem in reaching that goal. See T71366.
  1. Skins shouldn't vary on context. Skins (as envisioned by Vector/Monobook etc) are not supposed to vary on Context apart from when a user is logged in. They certainly shouldn't behave differently if opted into a beata mode. Of course, we could just setup varnish to vary the desktop site on beta cookie, but several years ago when there were talks about BetaFeatures being available to anons this was a non-starter from an ops pespective. Of course, we could have left an inline comment to not run this in stable, but I prefer to guard against potential future scenarios where it's easy to do so rather than make assumptions of how the status quo will never change.

@phuedx feel free to hijack T125588 - that bug is a little vague.

phuedx closed this task as Resolved.Nov 1 2016, 4:03 PM

I can confirm that the "(Beta)" suffix is added to the search bar when I've opted in to the mode.

phuedx added a comment.Nov 1 2016, 4:05 PM

@phuedx feel free to hijack T125588 - that bug is a little vague.

ACK