Remove non-React UI code #1891

Vinnl · 2022-05-04T16:19:18Z

This PR removes all code that no longer runs, because it was used to generate the non-React UI:

Unused Django partials and views, i.e. everything but wrapped_email.html. That said, there are still empty templates for home and faq, since there is Python code referring to those routes to generate their URLs. My intuition would be that just referring to those URLs directly would be a better solution, but I'll defer to Django authorities on that.
Gulp, Sass, ESLint (at the root, not in /frontend/) and stylelint (at the root, not in /frontend) are removed.
References to SERVE_REACT are removed, and React is now served by default.
The package.json at the root now refers to the one in /frontend as a workspace. The file is still used to tell Heroku how to build, and this ensures that you can also just run npm install at the root. This does also result in a big update to the package-lock.json at the root, which is the sole reason that this PR has more additions than deletions.
~~Images are now co-located with the code that's using them, as requested in reviews of The Big One.~~ → extracted to Co-locate images with their usage site #1998.

There's also an equivalent PR for the add-on, though it's independent from this one and needn't be merged at the same time.

How to test: basically, nothing should break anywhere!

This also fixes #1858 according to John.

l10n changes have been submitted to the l10n repository, if any.
I've added a unit test to test for potential regressions of this bug. N/A
I've added or updated relevant docs in the docs/ directory. (Onboarding doc still to be updated, after this is merged.)
All UI revisions follow the coding standards, and use Protocol tokens where applicable (see /static/scss/libs/protocol/css/includes/tokens/dist/index.scss).
Commits in this PR are minimal and have descriptive commit messages.

netlify · 2022-05-04T16:19:57Z

✅ Deploy Preview for fx-relay-demo canceled.

Name	Link
🔨 Latest commit	`64c57a3`
🔍 Latest deploy log	https://app.netlify.com/sites/fx-relay-demo/deploys/629639427b7455000805a34d

groovecoder

Changes (mostly removals) in the python look good. Let's leave the url-referenced no-op views and empty templates in place just to minimize the risk of knock-on effects in this change (which is already really large).

I'd like @maxxcrawford, @codemist , and/or @lloan to check thru the SCSS changes too.

groovecoder · 2022-05-04T19:27:21Z

frontend/src/components/landing/DemoPhone.tsx

+  let foreground = FgImage.src;
+  if (lang === "fr") {
+    foreground = FgImageFr.src;
+  }
+  if (lang === "de") {
+    foreground = FgImageDe.src;
+  }


Suggestion (non-blocking) kill let with fire ...

Suggested change

let foreground = FgImage.src;

if (lang === "fr") {

foreground = FgImageFr.src;

}

if (lang === "de") {

foreground = FgImageDe.src;

}

const foreground = lang === "de" ? FgImageDe.src : lang === "fr" ? FgImageFr.src : FgImage.src;

Though I think maybe in the past I asked to REMOVE a nested ternary expression? I guess I dislike let more than I dislike nested ternary expressions.

We can have neither using your old trick :) fe363f5

@Vinnl @groovecoder if you want to get crazy, you could do this too, lol.
const foreground = {"de": FgImageDe.src, "fr": FgImageFr.src}[lang] ?? FgImage.src;

privaterelay/urls.py

groovecoder · 2022-05-04T19:40:00Z

privaterelay/urls.py

-        path('premium', views.premium_promo, name='premium-promo'),
-        path('', views.home, name='home'),
-    ]
-


suggestion (non-blocking): Looks like we can also remove these entire view functions from privaterelay.views: profile, settings_view, settings_update_view, premium_promo. Since they are not bound to any urls.

Ah yes, good one: 42131f0

We still need any patterns that are called by name by reverse. The ones I see left in the code are faq, home, profile (privaterelay/views.py line 58), fxa_login, and relayaddress-detail. So we need profile back.

Sharp eye @jwhitlock! The reverse for the profile never gets called though, because the view it's in in turn only exists for the URL to the homepage, and never gets rendered. Possibly I could replace the remaining views by empty functions, i.e. removing this reverse call?

That said, it feels a bit weird to me to keep these views just to be able to call reverse on them to generate a URL - I think the idea behind reverse is to ensure links remain up-to-date even if URLs change? Which doesn't get achieved because the views don't represent "real" URLs. (Besides, URLs should remain valid anyway.) How would you feel about replacing all reverse calls with direct references to the URLs they end up at?

How would you feel about replacing all reverse calls with direct references to the URLs they end up at?

I think that's a good plan, ship it!

jwhitlock · 2022-05-05T20:52:30Z

privaterelay/urls.py

    path('faq', views.faq, name='faq'),
+    # This route is still referred to in middleware.py, so leave it in
+    # (even though the homepage is no longer rendered by Django):
+    path('', views.home, name='home'),


👍 fixes #1858

maxxcrawford

This PR is a tiny bit bittersweet 🥲

General Praise: Thanks for circling back on some requests from the Big One. That's some excellent callback!

As requested at:

The Big One #1518 (comment)

The Big One #1518 (review)

Note: This will be a two-part review. Having to stop at my EOD.

package.json

.eslintrc.js

frontend/src/components/Banner.module.scss

frontend/src/components/Button.module.scss

frontend/src/components/dashboard/Onboarding.module.scss

As requested at #1891 (comment)

As called out at #1891 (comment)

Vinnl · 2022-05-19T07:49:30Z

frontend/src/components/Icons.tsx

+/** Icon of a question mark, that inherits the text color of its container */
+export const HelpIcon = ({
+  alt,
+  ...props
+}: SVGProps<SVGSVGElement> & { alt: string }) => {
+  return (
+    <svg
+      role="img"
+      aria-label={alt}
+      aria-hidden={alt === ""}
+      xmlns="http://www.w3.org/2000/svg"
+      viewBox="0 0 16 15"
+      width={16}
+      height={15}
+      style={{
+        fill: "currentcolor",
+        ...props.style,
+      }}
+      {...props}
+    >
+      <title>{alt}</title>
+      <path d="M7.625 1.75c3.446 0 6.25 2.804 6.25 6.25s-2.804 6.25-6.25 6.25-6.25-2.804-6.25-6.25 2.804-6.25 6.25-6.25m0-1.25a7.5 7.5 0 1 0 0 15 7.5 7.5 0 0 0 0-15z" />
+      <path d="M7.625 9.709A.625.625 0 0 1 7 9.084l0-.767c0-.527.37-.987.879-1.092.577-.12.996-.635.996-1.225a1.252 1.252 0 0 0-2.432-.409.626.626 0 0 1-1.181-.408A2.5 2.5 0 1 1 8.25 8.421l0 .663c0 .345-.28.625-.625.625z" />
+      <path d="m8 12-.75 0-.25-.25L7 11l.25-.25.75 0 .25.25 0 .75z" />
+    </svg>
+  );
+};


Once #1883 is merged, this can be replaced by SupportIcon.

Vinnl · 2022-05-19T07:50:39Z

frontend/src/components/Icons.tsx

+/** Icon of a chat bubble, that inherits the text color of its container */
+export const MessageIcon = ({
+  alt,
+  ...props
+}: SVGProps<SVGSVGElement> & { alt: string }) => {
+  return (
+    <svg
+      role="img"
+      aria-label={alt}
+      aria-hidden={alt === ""}
+      xmlns="http://www.w3.org/2000/svg"
+      viewBox="0 0 16 15"
+      width={16}
+      height={15}
+      style={{
+        fill: "currentcolor",
+        ...props.style,
+      }}
+      {...props}
+    >
+      <title>{alt}</title>
+      <path
+        fillRule="evenodd"
+        clipRule="evenodd"
+        d="M2.423 0H13.577C14.9145 0.00165294 15.9983 1.0855 16 2.423V8.577C15.9983 9.9145 14.9145 10.9983 13.577 11H13V14C12.9999 14.4227 12.7341 14.7996 12.3361 14.9417C11.938 15.0837 11.4936 14.9601 11.226 14.633L8.26 11H2.423C1.0855 10.9983 0.00165294 9.9145 0 8.577V2.423C0.00165294 1.0855 1.0855 0.00165294 2.423 0ZM13.8761 8.87611C13.9554 8.79678 14 8.68919 14 8.577V2.423C14 2.18938 13.8106 2 13.577 2H2.423C2.18938 2 2 2.18938 2 2.423V8.577C2 8.68919 2.04457 8.79678 2.12389 8.87611C2.20322 8.95543 2.31081 9 2.423 9H8.734C9.03434 8.99975 9.31889 9.13449 9.509 9.367L11 11.194V10C11 9.44771 11.4477 9 12 9H13.577C13.6892 9 13.7968 8.95543 13.8761 8.87611ZM11.5 4H4.5C4.22386 4 4 4.22386 4 4.5C4 4.77614 4.22386 5 4.5 5H11.5C11.7761 5 12 4.77614 12 4.5C12 4.22386 11.7761 4 11.5 4ZM4.5 6H11.5C11.7761 6 12 6.22386 12 6.5C12 6.77614 11.7761 7 11.5 7H4.5C4.22386 7 4 6.77614 4 6.5C4 6.22386 4.22386 6 4.5 6Z"
+      />
+    </svg>
+  );
+};


Once #1883 is merged, this can be replaced by ContactIcon.

Vinnl · 2022-05-19T07:52:44Z

frontend/src/components/layout/UserMenu.tsx

+  HelpIcon,
+  MessageIcon,
+  NewTabIcon,
+  SettingsIcon,


For consistency with #1883, this should actually use Cogwheel.

maxxcrawford · 2022-05-24T20:18:26Z

@Vinnl Bleh. I know there's a ton of conflicts because this has been open for two weeks. Could you please catch this up (whenever you think is the best time of week to do this) to review/approve?

Thanks!

@jwhitlock mentioned this in the retro: If you wanted to break this up into separate, more focused/exploded PRs (not saying this one isn't), I'm happy to review that way too. Whatever works for ya!

As requested at #1891 (comment)

As called out at #1891 (comment)

Vinnl · 2022-05-25T13:53:50Z

@maxxcrawford Rebased and resolved conflicts, and I removed the parts that moved images next to where they were used → they're now in #1998.

As requested at #1891 (comment)

As called out at #1891 (comment)

Vinnl · 2022-06-09T15:31:09Z

Rebased and resolved conflicts again.

Vinnl · 2022-06-17T09:10:37Z

No more conflicts, but rebased again to be sure.

This removes most of the views rendered by Django, linters applying only to those files, and the infra to compile Sass. It preserves the package.json file at the root, primarily for our Heroku configuration (to set the Node and npm versions, and the commands to run to build the website - i.e. heroku-postbuild), but also to list the one in `frontend` as an npm workspace, leading to an npm install in the root also running frontend/'s postinstall scripts and thereby set up the Git hooks. I've left the routes to the Django-rendered FAQ and home pages in place (even though those views are now empty), because they're still used to generate links. It might be more sensible to just replace those with direct links, but I'll let people with more Django experience decide on that. I've also left the template tags in place, in case they still come in useful when e.g. rendering the forwarded email.

`reverse` is useful to maintain links even if URLs change (which they shouldn't!), but since we were maintaining views that never get rendered (because they're replaced by static HTML, not rendered by Django), that benefit no longer plays out.

Vinnl · 2022-06-27T11:26:57Z

No more conflicts, but rebased again to be sure.

maxxcrawford

Shout out to @Vinnl for continuing to keep this monster PR up to date.

I went back into the old texts and looked at my initial "Gulp"'ify'ing PR to see if everything was removed, and it all looks correct.

I did find one file path that was not modified/removed from that PR:

https://github.com/mozilla/fx-private-relay/blob/main/.buildpacks.

Not sure if its still needed for our React frontend, or if it can also be removed. Lmk!

I'm good with this chapter to be finally closed. 🙏

package.json

maxxcrawford · 2022-06-27T20:35:33Z

emails/templates/emails/wrapped_email.html

@@ -48,8 +48,7 @@
                  {{ email_wrap_forwarded_from|safe }}
                  {% if has_attachment %}
                      {% ftlmsg 'nav-faq' as faq_text %}
-                      {% url 'faq' as faq_url %}
-                      {% with SITE_ORIGIN|add:faq_url as faq_url %}
+                      {% with SITE_ORIGIN|add:"/faq/" as faq_url %}


Question: Do we need to do some end-to-end email testing to make sure the FAQ paths are correct with these changes or they already in place? (Reworded: Is this actually making any customer facing link change, or just a refactor with the same end-result?)

I think http://127.0.0.1:8000/emails/wrapped_email_test is enough to test this template properly.

Just a refactor with the same end result, and just verified another time with the wrapped_email_test template that it does indeed render correctly, to be sure.

maxxcrawford · 2022-06-27T20:37:26Z

privaterelay/templates/home.html

@@ -1 +0,0 @@
-{% include "newlanding/a/index.html" %}


Question for @codemist: Is there anything from the previous Django homepage that wasn't ported over to React?

groovecoder · 2022-06-27T21:20:22Z

I did find one file path that was not modified/removed from that PR:
* https://github.com/mozilla/fx-private-relay/blob/main/.buildpacks.
Not sure if its still needed for our React frontend, or if it can also be removed. Lmk!

Ooo, AFAIK we only needed the node engine on heroku for django-gulp. Could even streamline heroku builds to remove it.

jwhitlock

Thanks @Vinnl! ❤️

Vinnl · 2022-06-28T10:41:35Z

The buildpack is still needed I think, as the frontend is still built on Heroku, requiring Node (and of course Python for the back-end).

As requested at #1891 (comment)

As called out at #1891 (comment)

As requested at #1891 (comment)

As called out at #1891 (comment)

As requested at #1891 (comment)

As called out at #1891 (comment)

Vinnl requested review from groovecoder and maxxcrawford May 4, 2022 16:19

Vinnl self-assigned this May 4, 2022

groovecoder reviewed May 4, 2022

View reviewed changes

jwhitlock reviewed May 5, 2022

View reviewed changes

jwhitlock mentioned this pull request May 5, 2022

MPP-79: Switch to black code style #1857

Merged

maxxcrawford reviewed May 5, 2022

View reviewed changes

Vinnl added a commit that referenced this pull request May 6, 2022

Remove use of let

fe363f5

As requested at #1891 (comment)

Vinnl added a commit that referenced this pull request May 6, 2022

Re-add homepage to package.json

b4bf17d

As called out at #1891 (comment)

Vinnl commented May 19, 2022

View reviewed changes

Vinnl added a commit that referenced this pull request May 25, 2022

Remove use of let

dc54fc9

As requested at #1891 (comment)

Vinnl added a commit that referenced this pull request May 25, 2022

Re-add homepage to package.json

848a69f

As called out at #1891 (comment)

Vinnl force-pushed the MPP-1575-remove-redundant-ui branch from b4bf17d to 0018e22 Compare May 25, 2022 13:49

Vinnl mentioned this pull request May 25, 2022

Co-locate images with their usage site #1998

Merged

5 tasks

Vinnl added a commit that referenced this pull request May 25, 2022

Remove use of let

b481d13

As requested at #1891 (comment)

Vinnl added a commit that referenced this pull request May 25, 2022

Re-add homepage to package.json

9385af1

As called out at #1891 (comment)

This was referenced May 25, 2022

Refer directly to non-Django paths #1999

Merged

Add API key field and copy to clipboard logic on settings page #2012

Merged

Vinnl added the size:M a few days label May 31, 2022

Vinnl force-pushed the MPP-1575-remove-redundant-ui branch from efdc828 to 64c57a3 Compare May 31, 2022 15:50

Vinnl force-pushed the MPP-1575-remove-redundant-ui branch from 64c57a3 to a35e9a4 Compare June 9, 2022 15:29

Vinnl added Review: M Code review time: 1-2 hours and removed size:M a few days labels Jun 13, 2022

Vinnl force-pushed the MPP-1575-remove-redundant-ui branch from a35e9a4 to d4e9ee7 Compare June 17, 2022 09:10

Vinnl added 2 commits June 27, 2022 13:26

Refer directly to non-Django paths

b53f5af

`reverse` is useful to maintain links even if URLs change (which they shouldn't!), but since we were maintaining views that never get rendered (because they're replaced by static HTML, not rendered by Django), that benefit no longer plays out.

Vinnl force-pushed the MPP-1575-remove-redundant-ui branch from d4e9ee7 to b53f5af Compare June 27, 2022 11:26

maxxcrawford approved these changes Jun 27, 2022

View reviewed changes

jwhitlock approved these changes Jun 27, 2022

View reviewed changes

Vinnl merged commit 1b6c4e9 into main Jun 28, 2022

Vinnl deleted the MPP-1575-remove-redundant-ui branch June 28, 2022 11:04

Vinnl added a commit that referenced this pull request Jun 28, 2022

Remove use of let

7fcbf7a

As requested at #1891 (comment)

Vinnl added a commit that referenced this pull request Jun 28, 2022

Re-add homepage to package.json

d539fbd

As called out at #1891 (comment)

Vinnl added a commit that referenced this pull request Jun 28, 2022

Remove use of let

6b69abc

As requested at #1891 (comment)

Vinnl added a commit that referenced this pull request Jun 28, 2022

Re-add homepage to package.json

aaffc3a

As called out at #1891 (comment)

Vinnl added a commit that referenced this pull request Jun 28, 2022

Remove use of let

126b270

As requested at #1891 (comment)

Vinnl added a commit that referenced this pull request Jun 28, 2022

Re-add homepage to package.json

02d920d

As called out at #1891 (comment)

Vinnl mentioned this pull request Jun 29, 2022

Fix Heroku build #2140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove non-React UI code #1891

Remove non-React UI code #1891

Vinnl commented May 4, 2022 •

edited

Loading

netlify bot commented May 4, 2022 •

edited

Loading

groovecoder left a comment

groovecoder May 4, 2022

Vinnl May 6, 2022

lloan Jun 24, 2022

groovecoder May 4, 2022 •

edited

Loading

Vinnl May 6, 2022

jwhitlock May 24, 2022

Vinnl May 25, 2022

jwhitlock May 25, 2022

Vinnl May 25, 2022

jwhitlock May 5, 2022

maxxcrawford left a comment

Vinnl May 19, 2022

Vinnl May 19, 2022

Vinnl May 19, 2022

maxxcrawford commented May 24, 2022

Vinnl commented May 25, 2022

Vinnl commented Jun 9, 2022

Vinnl commented Jun 17, 2022

Vinnl commented Jun 27, 2022

maxxcrawford left a comment

maxxcrawford Jun 27, 2022

groovecoder Jun 27, 2022

Vinnl Jun 28, 2022

maxxcrawford Jun 27, 2022

groovecoder commented Jun 27, 2022

jwhitlock left a comment

Vinnl commented Jun 28, 2022

Remove non-React UI code #1891

Remove non-React UI code #1891

Conversation

Vinnl commented May 4, 2022 • edited Loading

netlify bot commented May 4, 2022 • edited Loading

✅ Deploy Preview for fx-relay-demo canceled.

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxxcrawford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxxcrawford commented May 24, 2022

Vinnl commented May 25, 2022

Vinnl commented Jun 9, 2022

Vinnl commented Jun 17, 2022

Vinnl commented Jun 27, 2022

maxxcrawford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder commented Jun 27, 2022

jwhitlock left a comment

Choose a reason for hiding this comment

Vinnl commented Jun 28, 2022

Vinnl commented May 4, 2022 •

edited

Loading

netlify bot commented May 4, 2022 •

edited

Loading

groovecoder May 4, 2022 •

edited

Loading