It might also be possible for the library to override the angle chosen (e.g. bottomRight) and use something else (e.g. right in this case) if it detects that the original would go off the screen.

swalling wrote:

Overriding the angle isn't a bad idea. It would be elegant to have it flip orientation. In the meantime, feel free to also just change the parsed item to a more neutrally-positioned one, for a short term fix for production.

Rather than trying to automatically compensate (which could be wonky), what if we allow tours to specify behavior in multiple skins?

Tours could keep using the current syntax that just expresses a single string, like:

attachTo: '#searchInput',
position: 'bottomRight',

So it's no more work at first writing a tour.

But for multiple skin support, also allow:

attachTo: '#searchInput',
position: {

default: 'bottomRight',
monobook: 'right'

}

We could also support:

attachTo: {

default: '#searchInput',
monobook: '#somethingElse'

},
position: {

default: 'bottomRight',
monobook: 'right'

}

In some cases, skins have different selectors for the same or equivalent thing. So it seems to make sense to implement both, working the same way.

It would first check for an override, but then fall back to to default (so you can customize for anywhere from none to all of the skins).

This is very similar to how ResourceLoader skinScripts and skinStyles work, including the 'default' fallback key.

tchay wrote:

I like this idea, but how do add support for this in a manner that can be upstreamed to Guiders?

There is already a wrapper method for initGuider (initializeGuiderInternal), which is used for internationalization, and making the button code at more high-level.

This particular one probably is not a candidate for upstraming (neither is the MW i18n-specific stuff).

However, the fix I'm about to do for bug 44804 (automatic flipping for RTL) is a candidate if they're interested. I'll probably do it in the wrapper for now, but I'll send upstream a note in case they think it would be useful there.

Do you think I should do something like:

position: {

skin: {
  default: 'bottomRight',
  monobook: 'right'
}

}

to try to be more future-proof, in case there's something else we need to fork on?

Or, keep it simple?

tchay wrote:

I think it's probably best to keep it simple (don't have a "skin") section, and avoid upstreaming it a la i18n changes. I know it's a small difference, but let's not make it more confusing.

If we do it this way and the user doesn't have a default but goes to the old style (where it's a string instead of an object) this will be handled fine right?

Yeah, I'm not going to upstream this part either way, since it doesn't make sense for other stuff.

Yes, it will handle the simple/old syntax.

Alright, I'll go with the simple approach. Ideally, there should be no other reason to distinguish (e.g. RTL flipping is now handled).

Fixed in https://gerrit.wikimedia.org/r/#/c/48790/ as originally proposed (simpler version without skin: key).

At Ori's suggestion, I changed default to fallback, so people don't have worry about reserved words.

This is now merged.

We decided to do the automatic flipping as well. See https://bugzilla.wikimedia.org/show_bug.cgi?id=45621 .