-
Notifications
You must be signed in to change notification settings - Fork 9
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
Allow scenarios for logged in users #2
Conversation
4f2f7f3
to
c3fd2b2
Compare
Latest patch seems more stable - using onBefore instead of onReady. |
ce41b5b
to
b76b2fd
Compare
Thank you for working on this. I'm still investigating the cause, but I've noticed the following error message 2 out of 3 runs of
|
src/engine-scripts/puppet/scroll.js
Outdated
@@ -0,0 +1,5 @@ | |||
module.exports = (page) => { | |||
page.evaluate( () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need await?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why but i was getting a timeout with this, presumably because it never resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, FINALLY worked this one out. I needed to return true in page.evaluate
Pushed new patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this with the version of the patch using onReady? I was getting those errors with that but not onBefore |
@jdlrobson Yes, it's with commit 91b4786 . But it might be due to the await question I posted above |
Sticky header shouldn't be showing on tablet view. Are you not seeing it on the desktop breakpoint? |
Ah good point. I see the following at the "desktop" breakpoint (not to be confused with the "desktop-wide" which shows the sticky header). Presumably at this breakpoint we shouldn't show the sticky header but it looks like our latest TOC styles are not equipped to handle this right now as there is a large gap where the sticky header would otherwise go: |
876ac4a
to
a82eade
Compare
@jdlrobson I tried an experiment with the last commit to see how much faster it would be to use an already retrieved cookie without an expiration (from my understanding). The tests are consistently 10-15 seconds faster compared to the previous commit which uses the login page. I really like the improvement in speed. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess since we're currently storing the password/username and there's no public instance of this, this should be okay. It would break however if the internals of how we log in change, but I guess this is fine for now.
No description provided.