task(settings): Full production rollout of react email-first#18747
task(settings): Full production rollout of react email-first#18747vpomerleau merged 1 commit intomainfrom
Conversation
vpomerleau
force-pushed
the
FXA-11221-fullProd
branch
3 times, most recently
from
555d85b to
fd56adc
Compare
vpomerleau
changed the title
| const isFirefox = /firefox/i.test(ua) || /FxiOS/i.test(ua); | ||
|
|
||
| let userFromBrowser; | ||
| if (isFirefox) { |
There was a problem hiding this comment.
I'm a little confused by this... is it in this PR because we cannot have it before the full react email-first rollout?
There was a problem hiding this comment.
This can be separated from this PR, I'll issue it separately.
There was a problem hiding this comment.
Feel free to leave it. I was confused in the literal sense; I was not able to make the connection between the full rollout and that.
Do we need to call this out for QA at all? Or is it going to be part of their tests anyway?
There was a problem hiding this comment.
These changes could be done separately (and there is a separate ticket - https://mozilla-hub.atlassian.net/browse/FXA-11520), but I'd ended up pulling them in because the changes were needed to get the tests working for full prod rollout.
One bit of the other Jira issue that is not covered here is checking if the environment matches before sending up a webchannel message (e.g., webchannel fails if running localhost without the dev launcher) - I'm not sure if that's fully needed. If there's a mismatch, there would just be a short delay (and warning in console) when loading the app but no error. What do you think?
Good callout about qa, I'll label the ticket qa+ and will add some notes for QA.
|
|
||
| if (userFromBrowser && userFromBrowser.sessionToken) { | ||
| const ua = navigator.userAgent; | ||
| const isFirefox = /firefox/i.test(ua) || /FxiOS/i.test(ua); |
There was a problem hiding this comment.
I don't think the match needs to be case insensitive. And I think string.includes should work.
| const metricsEnabled = useMemo(() => { | ||
| if (metricsLoading || !integration || isSignedIn === undefined) { | ||
| return; | ||
| return undefined; |
There was a problem hiding this comment.
Is there an advantage in making the undefined explicit here?
There was a problem hiding this comment.
Not really, I'll revert this bit
vpomerleau
force-pushed
the
FXA-11221-fullProd
branch
2 times, most recently
from
ea125de to
fac2ec2
Compare
Because: * We want to route all traffic to the react app version of the email-first pages This commit: * remove these pages from the experiment * Set fullProdRollout to true for email first routes * Update navigation to stay in react app (no hard navigation) * Update tests Closes #FXA-11221
vpomerleau
force-pushed
the
FXA-11221-fullProd
branch
from
fac2ec2 to
96efced
Compare
2 participants
Because
This pull request
useNavigateWithQueryto make it clearer that params will persist (no renaming touseNavigate/navigate)Issue that this pull request solves
Closes: FXA-11221
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Note to reviewers: Recommend restarting the app locally with these updates and doing some exploratory testing with dev launcher