Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MWPW-141022 [Project PEP] Prompt Dismissal + Tie-in with App Launcher UX #2392
MWPW-141022 [Project PEP] Prompt Dismissal + Tie-in with App Launcher UX #2392
Changes from 41 commits
6b9a0bd
9e95af6
2ec9eff
601929d
c41777b
15f1339
5b4c5e8
f5df88d
332c265
538f5d3
565e1f9
4ab6a2f
e9a2143
b29f5f4
bb9d6ef
fe91f40
38043a9
2941bf9
7777616
006a656
24820f9
114e6e1
7d305c7
470a6db
f4a0192
a635600
52b18b1
aefd866
663641c
cda1018
3e0a806
ffb6a1f
baf08e6
4449fed
b927473
0378d4f
a95f74b
7077764
fb3468b
d5d6cd9
319d276
160c166
b727d52
9606978
e2c99c3
2fd0567
15e7363
c640167
538c9eb
afcd4ad
2343413
32504b2
99c5ec7
d4b68ec
8d3e25a
b2b0577
fb508bd
a995f25
d47915d
e2c89f7
1f64afb
d8348e1
f332e8a
1e196b3
e2068f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why not just define
padding-inline
andpadding-block
, since you're defining both values for each?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.
This is used in multiple places, it should be a scoped variable
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'd just missed deleting this one
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.
This is already defined in the individual
:after
/:before
definitions, does it need to be defined here as well?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.
Couldn't this be just
0
?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'd kept them like this so I could fiddle with them. I've made it into a variable.
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.
Are the newlines on purpose on the lines:
205, 210, 211, 227, 244, 279, 283, 288
?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.
Also there is a mix of tabs and spaces. All should be spaces.
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've fixed both the issues (some of the newlines were on purposes; I replaced each tab with the appropriate number of spaces)
Check warning on line 56 in libs/features/webapp-prompt/webapp-prompt.js
libs/features/webapp-prompt/webapp-prompt.js#L55-L56
Check warning on line 90 in libs/features/webapp-prompt/webapp-prompt.js
libs/features/webapp-prompt/webapp-prompt.js#L89-L90
Check warning on line 260 in libs/features/webapp-prompt/webapp-prompt.js
libs/features/webapp-prompt/webapp-prompt.js#L260
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.
Could you test this by defining a hash as the redirect URL? That would avoid the page reload, but would trigger the UI changes.
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.
Unfortunately this still results in this line ostensibly never being run. It's difficult to say why, but it may have something to do with how fakeTimers interacts with the event loop.n
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.
Could these not be exported from the main logic file and imported here to avoid duplication?
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.
super nit: verbs to start test description statements:
adds three rings to the app switcher and removes them after the allotted time
. You can always addshould
to the start of test descriptions because our code always should do something.