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

Jump to bottom

PCS: expose pagelib CSS #1048

Merged
merged 3 commits into from Aug 21, 2018
Merged

PCS: expose pagelib CSS #1048

merged 3 commits into from Aug 21, 2018

Conversation

berndsi
Copy link
Contributor

@berndsi berndsi commented Aug 20, 2018

@berndsi
PCS: expose pagelib CSS
1a17406 
http://localhost:7231/en.wikipedia.org/v1/data/css/mobile/pagelib

Bug: https://phabricator.wikimedia.org/T202105
@berndsi berndsi requested a review from Pchelolo August 20, 2018 22:25
@Pchelolo
Copy link
Contributor

What prevents us from merging the 2 and make {type} a parameter with enum and 2 possible values base or pagelib?

@berndsi
Copy link
Contributor Author

berndsi commented Aug 21, 2018 •
edited

What prevents us from merging the 2 and make {type} a parameter with enum and 2 possible values base or pagelib?

Besides that we would probably need to do this for all three variations (base, pagelib, site), not much. There are some minor differences in the summary, description, and the x-amples.title fields. I think the latter could be just two (or three) x-amples.

@Pchelolo
Copy link
Contributor

The latter can be just several examples - easy!

The former - we can just make a combined description with something like a bullet-point list with different variants.

I think it would be nicer then a giant copy-paste

@berndsi
Copy link
Contributor Author

berndsi commented Aug 21, 2018

Ok, will give it a shot.

@Pchelolo
Copy link
Contributor

👌

@berndsi
PCS: make CSS generic
7fd498c 
The three CSS endpoints are similar enough to make it generic and have
a parameter `file` for which kind of CSS is used:
base, pagelib, or site.

QA:
http://localhost:7231/en.wikipedia.org/v1/data/css/mobile/base
http://localhost:7231/en.wikipedia.org/v1/data/css/mobile/pagelib
http://localhost:7231/en.wikipedia.org/v1/data/css/mobile/site

Bug: https://phabricator.wikimedia.org/T202105
@berndsi
Copy link
Contributor Author

berndsi commented Aug 21, 2018

CI's still not happy but a bit better than before. Still, all seem unrelated issues. This time it's Node 6 with a PDF service 500.

@Pchelolo
Copy link
Contributor

I'm almost inclined to make the PDF tests just a warning. They're too annoying

Pchelolo
Pchelolo reviewed Aug 21, 2018
View reviewed changes
Copy link
Contributor

@Pchelolo Pchelolo left a comment

Choose a reason for hiding this comment

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

Looks good, some bikeshedding opportunity inlined, feel free to ignore

v1/css.yaml Outdated
@@ -11,20 +11,37 @@ info:
name: Apache licence, v2
url: https://www.apache.org/licenses/LICENSE-2.0
paths:
/css/mobile/base:
/css/mobile/{file}:
Copy link
Contributor

Choose a reason for hiding this comment

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

Bikeshedding... Maybe 'type' or smth?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, while this does get a file, in the context of the API type sounds more appropriate. Let's change that, otherwise LGTM.

Copy link
Contributor Author

@berndsi berndsi Aug 21, 2018 •
edited

Choose a reason for hiding this comment

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

Ok, I could go with that. Another idea I had would be bundle. What do you think of that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I could go with that. Another idea I had would be bundle. What do you think of that?

This is a totally bikeshedding session here, but I personally don't like bundle - the bundle is what's returned, and the parameter for the bundle is the type of the bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, type it is then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

d00rman
d00rman requested changes Aug 21, 2018
View reviewed changes
v1/css.yaml Outdated
@@ -11,20 +11,37 @@ info:
name: Apache licence, v2
url: https://www.apache.org/licenses/LICENSE-2.0
paths:
/css/mobile/base:
/css/mobile/{file}:
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, while this does get a file, in the context of the API type sounds more appropriate. Let's change that, otherwise LGTM.

@d00rman d00rman merged commit 506c34d into wikimedia:master Aug 21, 2018
@berndsi berndsi deleted the pagelib-css branch August 21, 2018 17:26
wmfgerrit pushed a commit to wikimedia/mediawiki-services-mobileapps that referenced this pull request Aug 22, 2018
@berndsi
mobile-html: also request pagelib CSS
734b29e 
Adds a new
<link rel="stylesheet" href="https://meta.wikimedia.org/api/rest_v1/data/css/mobile/pagelib">
to the html/head element.

Note: the required RESTBase PR[1] was deployed a few hours ago.

QA: http://localhost:6927/en.wikipedia.org/v1/page/mobile-html/Dog

[1] wikimedia/restbase#1048

Bug: T202105
Change-Id: Ifd68b5344b4da04d900ec178baf1b37e6a37c47d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pchelolo Pchelolo Pchelolo left review comments

@d00rman d00rman d00rman approved these changes

Assignees
No one assigned
Labels
API enhancement
Milestone
No milestone
3 participants
@berndsi @Pchelolo @d00rman