-
Notifications
You must be signed in to change notification settings - Fork 2
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
Pre-merge review #6
Comments
Even though we are refreshing the page every 60 seconds, we've configured the PWA plugin from Weston Ruter to store the REST API data with an expiration time of 15 minutes. This means all requests are served from the browser cache (localStorage or whatever that plugin uses to locally store data). You can see that here. Therefore, I don't think this as a blocker IMHO. And if 15 minutes is still a problem, we could always increase the time...
The
That's something we can discuss, but to me this is not a blocker. Let me explain it. The #design team of WCEU sent us this design you can see here. That's what we ended up creating. So, what should we do here?
Is this an issue with the PWA plugin? I don't think our JS code produces a memory leak because it is quite simple. What do you think?
That part was developed by @tfrommen and/or @iceablemedia. Maybe they have something to say about it...
IIRC we where using the same markup. At least I tried to keep it close to the source code I was given here and here. But maybe I did it wrong ;-)
Agree
Agree
The URL we use when in production is the URL of the site where the plugin is installed. We already have this under control IMHO.
Agree
No idea about that :-D
Agree
I did that because some attributes (title.rendered with weird characters) in the REST API have characters that didn't show properly unless they were properly encoded. |
Ah, that's right. But it still seems like there's a problem. If the caching were working then I'd expect the requests that happen every 60 seconds to resolve immediately, and maybe have something in the response headers to indicate that they were resolved from cache. Instead, they're taking 300+ ms, so they appear to be hitting the live server every time. Once that's working, so that each user only hits the server every 15 minutes, that should be fine from a scaling perspective. From a user perspective, though, that's still a lot of cellular data (almost 10MB per day if the user checks twice an hour for an 8 hour camp). It's also important to consider how slow that will load on a 2-3G connection (which is still very common IIRC). So, to me it still seems worth pairing down to only the fields that are used.
Doh 🤦♂ , I should have realized that. Yup, that's working fine :)
It seems like a problem with the design to me. Maybe it was just an oversight? If it was intention, what's the reasoning behind it? ( I'm guessing we'll need to ping one of the designers, but I'm not sure who they are )
I wouldn't assume that just because it's simple that there isn't a leak. They're uncommon (or at least not commonly noticed), but not necessarily hard to create. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management My first guess would be that it's related to I commented
The markup is close, but the CSS is using Flexbox instead of Grid. It's probably not worth worrying about this or the BEM classes until the Schedule block is finished, though. At that point it'll be much easier to see how the two can be integrated and hopefully reuse code/styles.
I'm not sure what you mean. If it's hardcoded, it will only work for WCEU 2019, but pretty soon we'll need to it work for all camps, so it'll need to use
Huh, that's odd. Do you remember an example of a post where that was happening? |
The way to do this would be to send an A good
This won't work as is because of wordcamp-pwa-page/includes/offline-storage.php Lines 52 to 57 in 0a49ba9
If this were changed to |
Changed in d7d673b |
I'm unable to reproduce the issue on Chrome or Firefox... It works fast on both even increasing the speed to 6000 milliseconds as you did.
Take a look at this line. If we are in development we use
IIRC there were problems with characters like the one in the name of this speaker. |
I was thinking about idea to use get_header and get_footer. Instead I simply put code in that will use main nav under top line. |
I would hope that this page would would be just like any other page on the WCEU site. So it should have the header and footer. The fact that the service worker is serving it from a cache in case of being offline should largely be irrelevant. The theme assets are already being cached via: wordcamp-pwa-page/includes/offline-storage.php Lines 35 to 47 in 0a49ba9
And the pages themselves are being cached with a stale-while-revalidate strategy: wordcamp-pwa-page/includes/offline-storage.php Lines 19 to 24 in 0a49ba9
So I should think that this new template should be just like any other template on the site. The only exception here is that it is able to fetch cached REST API responses to populate the schedule. In order for users to be able to navigate to the schedule page, I suggest adding an It is easy to define such a template: https://github.com/westonruter/twentyseventeen-westonson/blob/master/offline.php Actually, perhaps the schedule should be embedded right into this It might be easier of this functionality were just added to the WCEU theme itself, rather than a separate page plugin. |
I'm happy to help configure the theme to integrate. |
How many hours did you leave it running for?
Ah, I see what you mean. Yeah, this is something I'll need to do, rather than you. I'll leave it on the checklist so it isn't missed, but I'll take care of it.
Huh, that's just a normal diacritic AFAIK, so there shouldn't be any problems with displaying it like any other character. https://2019.europe.wordcamp.org/wp-json/wp/v2/speakers/5960 I guess one potential difference is that the HTML page is output with UTF-8 encoding, and the So yeah, it does seem like something is needed there, although manually re-encoding it in the markup every time there's a field that might include a diacritic doesn't seem like a sustainable solution. I'm guessing other people have run into this before; what solutions did they come up with? |
That wouldn't be necessary if we reused the existing footer, though. Although, I can see a use case for wanting to have different widgets on this version of the home page. 🤔 I definitely agree with Weston that, conceptually, the day-of template is completely unrelated to PWAs, and is for all users, not just mobile. (During the merge, I'm actually planning on separating the offline stuff into an mu-plugin, and the template stuff into a "wordcamp-day-of" plugin, to avoid conflating the two, and to have things named correctly.) Despite that, I can see an argument that we'd want a stripped-down version of the entire homepage during the event, so all visitors can focus on the essential content regardless of their device, but it does mostly feel wrong to me to have completely different headers/footers. I'm leaning towards it being best to be consistent across pages, but would like to keep the discussion going, in case there's a good compromise or something we haven't thought of yet. The example of having different footer widgets does seem like a compelling reason to have at least some kind of adapation, but it's definitely not essential IMO.
Do you mean in terms of file size, or dimensions? If it's the file size, that's a problem for people on all devices and across all pages, and I think the proper solution for that is to use a smaller image. It looks like it's 118k, which is a bad practice IMO. That's up to the individual WordCamp to decide, though. I don't think it's something we should take into account when making decisions here. It should probably also output the image with Or do you mean the image dimensions? It should be responsive on a small screen, and could even be hidden via the day-of template's CSS. I'd personally love to see header images disappear from the Web, since they're just eye candy and often hurt performance, but that's really best left to each camp to decide, so also not something we'd want to take into account here IMO. Individual camps can hide the image on the day-of template if they'd like, but I don't think we'd want to force that. |
That's a great idea! I added it to the stretch goals section.
I wonder if we'll want to do that anyway, since the schedule is something that most site visitors will want to visit at some point. 🤔
All of this functionality needs to work in mostly the same way for all camps, regardless of which theme they choose, so I don't think that's an option. |
Thing is that design for this page and other pages was different, so I was looking for best approach to that. While I do agree with you, also I understand a need that this page might be slightly different than other pages will be, as others are mobile-friendly, this is mobile-first.
In this particular case - both. Not giving an option to use images was, to me, better than left to org designers understanding the purpose of that page :) |
I realized that the design is missing the location information, which seems very critical. Attendees making their way to the venue before the event starts will need the address, the start time, the date, a static map image, etc. I'd normally consider that a pre-merge blocker, but since we're so late in the process I added it as a stretch goal instead. It's definitely something we'll want before activating it for all camps, though. It should be fairly easy to add, though, especially since it'll also be used for the offline template (see #9), so we can just create a single template partial can both templates can be reused. |
I think that's a really good point, that the needs of this page are different than the needs of the other pages. Although I do see the purpose somewhat differently; I think the goal here is to make the most relevant content available to all visitors, regardless of their device. Some attendees will have laptops and large tablets at the event, livestream visitors will be using desktops, etc. I looked at various sites, and it doesn't seem like the header area would be a problem for most. For WCEU 2019 the content is still above the fold. Sites that use header widgets would push the content below, though, which definitely isn't ideal for this situation. I wonder if there's a way that we can achieve the goal without the unintended side-effects, like maybe the day-of template uses CSS to hide the header image & widgets by default? That would preserve the ability of organizers to restore them if they feel like it's best for their situation. I don't think there's any harm in using the regular footer, since it's at the bottom. I posted this to the Community p2 to get a wider range of feedback than we'll be able to get on GitHub: https://make.wordpress.org/community/2019/04/30/wordcamp-pwa-an-update/#comment-26927 |
I'll take this one to ease in, and work my way up to the harder stuff. Here's what I had in mind:
|
😆, definitely :) ...but we also can't ship something that's causing the browser to freeze for 50% of the people who've tested it :) It may be that this is fixed by having the service-worker intercept the requests and return the cached version until 15 minutes have passed, but I'm still seeing the network requests take 300+ ms, even after d7d673b. Although, now that I think about it, that's probably because {
session: undefined
track: Object { id: 46, count: 4, link: "https://2016.misc.wordcamp.test/track/foo/", … }
<prototype>: Object { … }
} |
@avillegasn: I'm also seeing @vdwijngaert, I opened #10 to discuss the loading state, since this thread is getting really long. Let's create new issues for any new topics, but we can continue using the checklist in the OP as a tracking thread. Everyone, If y'all think it'd be easier to have separate issues for the header/footer styles, memory leak, etc, please feel free to create them and we can move the discussions there, instead of making this longer. Then this can just serve as a tracking thread. |
@avillegasn, in addition to the people in this thread, can you give me a list of people who should get |
@mburridge and, last but not least, @iandunn 🥇 :-D |
Thanks! It'll be will be merged to
|
Edit: Moved this comment to #13 to keep this thread manageable |
Hey everyone, this is running on WordPress/wordcamp.org@80baf45...42a361e I refactored a lot of it in order to integrate it into the wordcamp.org codebase, rather than being a standalone plugin. It's still pretty rough around the edges, but should work similar to the prototype. Please test it out to make sure I didn't miss anything, though! I drafted a page at https://2019.europe.wordcamp.org/?page_id=11378&preview=true, and you can override the current time by setting I removed all the WCEU-specific CSS, since the features are meant for all camps, so you'll need to add whatever custom CSS you'd like to your Remote CSS stylesheet. If there are any problems during the event, @vedanshujain will be in attendance and can help troubleshoot. Pinging him on the w.org Slack might be the best way to get ahold of him (Vedanshu, correct me if I'm wrong). If you can't get ahold of him, Andrea, Josepha, Hugh, Rocío, and Angela will also be there, and have my cell phone number. It's worth noting that the Session CPT doesn't yet track the length of the session, so if you set "now" as For any future changes, please submit PRs to https://github.com/WordPress/wordcamp.org rather than this repo, since the production repo has some significant differences, and is what production runs. |
Woohoo! I was super happy to see the Add to Homescreen prompt when I visited the WCEU site! It's now on my homescreen and I can confirm that visited pages continue to work when in airplane mode 🎉 Following up on #6 (comment): Will a themed Code to add the template from a plugin can be found at https://gist.github.com/westonruter/900611f075374c5468ea492f23ad4810 As long as this template includes Makes sense to address this later as well. Not critical for launch. |
Happy to see it merged! Hey @iandunn could you please change the strings back to what they were originally? I'm not a native English speaker but members in the Attendees Services Team that are native told me that the 'On now' and 'Up next' strings were more direct and are a best option here. I opened an issue in the meta repo: WordPress/wordcamp.org#127 Thanks for your work 👍 |
I've got that partially working locally, but didn't have time to commit it yesterday. Hoping to get it close enough to deploy today, though.
That's what I'm hoping. I ran into some problems w/ the stylesheet not being cached properly, but it might need a precaching route registered for that, even though the service worker should be caching it. There's also Custom/Remote CSS that's outside For the markup differences between themes, I'm hoping we can solve that w/ some default styles that target the various themes, but we'll see how complex that gets. I may end up just deploying a hardcoded template today, and we can integrate |
@avillegasn , just to make sure we have the same assumptions, this is how I imagined you launching the day-of template:
Let me know if you see any problems with that. |
this is exactly what we planned on doing early tomorrow in the morning. Good to see we all agree on this! 👍 I'm actually going to do it now to ensure it works as expected. Considering it is close to midnight here, contributor day is well over and doesn't need to be on the front page anymore ; and it's not too early to have tomorrow's first session appear as the next thing to happen. |
This is now done. Seems to work as expected so far. Finger crossed that it will work as expected for the next two days! |
Glad to hear it! If you notice anything going wrong, please reach out to Vedanshu and we'll do our best to resolve it. |
Noting here that I found an issue that the full blog (like what used to be the homepage) was no longer available anywhere on the site - this meant that clicking the 'view all posts' button just refreshed the homepage and showed the 'day of' template again. I fixed this by creating a new page called "News" and setting that as the 'Posts page' in Settings > Reading, so now that button links to that page automatically (although Chrome tends to cache pretty aggressively so if you visited the PWA with the 'day of' template already then it might not link to that until the cache clears). You can see it here: https://2019.europe.wordcamp.org/news/ You can change the name/slug of that page by simply editing the page here: https://2019.europe.wordcamp.org/wp-admin/post.php?post=11464&action=edit Apologies if this feels like over-stepping in terms of what I should be doing, but I woke up early (around 4am) and noticed it as a pretty serious issue and thought this would be the quickest way to fix it, and I figured that it's very easy for anyone to change the page name/slug at any point so it's a low-risk quick win :) |
Moved this to WordPress/wordcamp.org#129 and WordPress/wordcamp.org#130 to finish the remaining work |
I tested it out and everything looks pretty good. I think we're on track to merge in time.
These are the remaining items that I think should be fixed before then. Let me know if you disagree about anything, I'm always open to discussion.
Pre-merge blockers
TypeError: Object(...) is not a function at api.js:16
whenisDevelopment
is false. Probably need to re-apply 3003a3e#diff-6ffcb43d3693ef6de357a6c8861753ceL4I'm worried about scaling with potentially thousands of visitors pulling down a large amount of data every 60 seconds. It'd be good to cache the result of the MySQL queries that are used to build the REST API response, and only refresh that transient when posts are published/updated. At first glance it seems like
rest_pre_dispatch
could be used.We're currently pulling ~615k of data every 60 seconds, which will use a ton of cellular data. That should be paired down to only the fields that actually get used.
Rather than pulling down fresh data every time, even when it hasn't changed, can we detect when it's changed and only pull new data then? IIRC service workers provide a way to do that.
Depending on how impactful the two changes above are, we might also want to consider something like Avoid loading images on slow connections #5
Is the
#on-now
content being rendered in your tests? It's not in mine, but I haven't dug into why.#up-next
looks fine.The navigation menu needs to be included, otherwise the user is stuck (assuming that this is set as the home page, but even if it's not it'd still be really bad UX to make the user navigate to home in order to access the menu). See the stretch goal below for some things that might impact how this is implemented.
Fatal errors when unexpected data encountered. e.g.,
Cannot read property 'link' of undefined at Session (session.js:15)
. Should gracefully handle cases where rest API response is an error, is an empty array, etc.Pre-merge stretch goals
These would be really nice to get sorted out before the merge, but could be done after if we run out of time.
Add location information above the live schedule, see Offline / error templates #9 for details about obtaining it. Create a template partial that both templates can reuse.
There's a memory leak or something somewhere. If you leave the page open long enough, the browser becomes very slow, and eventually the thread is unresponsive.
I'm not entirely sure either way, but I'd lean towards having the PHP template call
get_header()
andget_footer()
, like a normal template, and just override the content area, rather than overriding the entire page. It seems better to have it preserve as much as possible, especially critical things like the navigation menu. That gives users a consistent experience, and gives organizers more control over the header/footer. One potential problem is wanting dedicated header/footer widget areas that only show up during the event, though. I don't have any great ideas off the top of my head. cc @coreymckrillProgramatically add a theme-agnostic
offline.php
template which contains the Location page contents, and the Schedule page contents. See Offline / error templates #9The plugin should just have basic theme-agnostic default styles. Anything that a camp wants on top of that should be in their custom CSS.
Test that the functionality works across all themes, not just CampSite 2017.
Firefox console is cluttered with detailed logging messages from
workbox
. Workbox debug messages clutter console GoogleChromeLabs/pwa-wp#170Use
wordcamporg
text domain.Rename variables and properties to be descriptive. e.g. of non-descriptive:
data
,track.track
Post-merge
These are things that are blockers for enabling the plugin for all camps (including WCEU 2020), but don't need to be done before the WCEU 2019 beta test.
Use the same markup/styles as the new Schedule block (wait until block is done).
Convert to BEMish classes - See coding standards and Class names don't follow convention WordPress/wordcamp.org#96 (wait until Schedule block is done)
Show
Loading...
or a spinner in the containers before JS renders themGenerate the REST API URL dynamically rather than hardcoding. (Ian will do during merge)
The URL to the Schedule page needs to be determined dynamically, rather than hardcoded.
Figure out what to do about the existing Day-Of page template and widget areas in CampSite theme. Not sure exactly what the best way is. Maybe we should deprecate them since this serves the same need, but across all themes and camps? This will probably be influenced by the decision on the stretch goal above to some extent. cc @coreymckrill
It seems like
fetchposts()
should be independent offetchsessons/tracks()
, rather than being combined in aPromise.all()
? Sessions and tracks are dependents on each other, but posts doesn't seem to be.It didn't occur to me earlier, but now I'm not sure we actually need
stripTagsAndEncodeText
. React will HTML-encode everything. Was the intention here to actually prevent valid/safe HTML from being used, or only to protect against XSS?The text was updated successfully, but these errors were encountered: