Field edit page with preliminary appearance tab #3661

mbertrand · 2022-07-28T22:05:00Z

Pre-Flight checklist

Screenshots and design review for any changes that affect layout or styling
- Desktop screenshots
- Mobile width screenshots
Migrations
- Migration is backwards-compatible with current production code
Testing
- Code is tested
- Changes have been manually tested

What are the relevant tickets?

Partially addresses #3632

What's this PR do?

Adds a form for editing field title and description, but not banner and avatar. These will be added in a separate PR.
Adds a tabbed edit page with 3 tabs (basic, appearance, moderators). All but appearance tab are just placeholders right now.
Creates a separate component for field avatar
Adds is_moderator to the API response to determine if the current user can access the field edit page/forms.

How should this be manually tested?

As an admin, go to the url /infinite/fields/<your_field_name>/manage/ and verify that you see 3 tabs.
Go to the "Appearance" tab and verify the title and description input fields are prepopulated with current values.
Make both blank and verify you see validation errors
Enter new values and submit. You should be redirected back to the field page and see your changes there.
As a non-admin/non-moderator your field, return to the edit page. You should see a message that you're not allowed to access the page.

Screenshots (if appropriate)

ChristopherChudzicki

I was just skimming this, two small comments.

frontends/infinite-corridor/src/libs/react-query.ts

codecov-commenter · 2022-07-29T20:20:16Z

Codecov Report

Merging #3661 (7feba81) into master (bc70f1b) will increase coverage by 0.03%.
The diff coverage is 92.56%.

@@            Coverage Diff             @@
##           master    #3661      +/-   ##
==========================================
+ Coverage   87.59%   87.62%   +0.03%     
==========================================
  Files         286      291       +5     
  Lines       12837    12938     +101     
  Branches     2115     2128      +13     
==========================================
+ Hits        11244    11337      +93     
- Misses       1369     1371       +2     
- Partials      224      230       +6

Impacted Files	Coverage Δ
frontends/infinite-corridor/src/libs/axios.ts	`100.00% <ø> (ø)`
open_discussions/settings.py	`94.19% <ø> (ø)`
channels_fields/api.py	`92.00% <66.66%> (-8.00%)`	⬇️
...rridor/src/pages/admin/EditFieldAppearanceForm.tsx	`86.95% <86.95%> (ø)`
...s/infinite-corridor/src/components/FieldAvatar.tsx	`88.23% <88.23%> (ø)`
...nfinite-corridor/src/pages/admin/EditFieldPage.tsx	`93.10% <93.10%> (ø)`
channels_fields/admin.py	`100.00% <100.00%> (ø)`
channels_fields/models.py	`97.05% <100.00%> (+0.08%)`	⬆️
channels_fields/permissions.py	`94.11% <100.00%> (-0.33%)`	⬇️
channels_fields/serializers.py	`95.12% <100.00%> (+0.18%)`	⬆️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.tsx

ChristopherChudzicki

As mentioned: I have to run but this looks good! I left some comments/suggestions.

ChristopherChudzicki · 2022-08-02T18:18:11Z

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.tsx

+    return editFieldChannel(field?.name, data)
+  })
+
+  const appearanceSchema = Yup.object().shape({


Requested Change: Move this outside the component function definition.

In general, when an object doesn't depend on the component parameters, it's probably better to define it outside of the component. Otherwise it's a new object on each render, and can cause react to over-eagerly re-render stuff.

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.tsx

ChristopherChudzicki · 2022-08-02T18:25:41Z

frontends/infinite-corridor/src/pages/FieldPageSkeleton.tsx

+            {field.data ? (
+              <>
+                <FieldAvatar field={field.data} imageSize="medium" />
+                <h2>{field.data.title}</h2>
+              </>
+            ) : null}


Minor Suggestion: Use condition && <SomeComponent /> when conditionally rendering a component instead of the ternary.

frontends/infinite-corridor/src/pages/admin/FieldAdminApp.tsx

ChristopherChudzicki · 2022-08-02T18:42:59Z

frontends/infinite-corridor/src/pages/admin/EditFieldPage.test.tsx

+    expect(
+      await screen.findByText("You do not have permission to access this page.")
+    ).toBeInTheDocument()


I am slightly conflicted about this suggestion, because I like how explicit expect is. But...

Suggestion: Just do await screen.findByText(...) without the expect.

because findBy... and getBy... both query the document body (so the result is guaranteed to be in the document, not some crazy detached DOM node) and they both throw if no results are returned. (Well, getBy throws, and findBy returns a rejected promise).

Reminder:

getBy = synchronous + throws if no result ... getAllBy also throws if 0 elements found

findBy = asynchronous + rejects if no result ... findAllBy also rejects if 0 elements found

queryBy = synchronous + can return null ... queryAllBy can return an empty list

ChristopherChudzicki · 2022-08-02T18:53:05Z

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.test.tsx

+import * as factory from "../../api/fields/factories"
+import { urls } from "../../api/fields"
+
+describe("EditChannelAppearanceForm", () => {


Question: I know fields are channels, so maybe the name is intentional, but I just wanted to double check.

Changed to EditFieldAppearanceForm

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.test.tsx

ChristopherChudzicki · 2022-08-02T18:56:46Z

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.test.tsx

+    titleError = await screen.findByText("Title is required")
+    expect(titleError).toBeInTheDocument()


As mentioned above, find will reject, so this can just be

Suggested change

titleError = await screen.findByText("Title is required")

expect(titleError).toBeInTheDocument()

await screen.findByText("Title is required")

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.test.tsx

ChristopherChudzicki · 2022-08-02T19:29:54Z

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.tsx

@@ -0,0 +1,131 @@
+import React from "react"


Rushed comment since I have to run:

Would it make sense to put this component in pages/admin? I do not have a strong feeling.

forms/admin?

But I guess all the forms for infinite will be admin forms

My thought was along the lines of co-locating the admin-editing code: pages/admin directory would contain the significant components that it uses, would import some stuff from ol-util or ol-forms. To make pages/admin more self-contained.

I.e., along the lines of this article Ferdi linked a while ago: https://alexmngn.medium.com/how-to-better-organize-your-react-applications-2fd3ea1920f1 Note: If you do read that, its examples are a bit outdated. Like, people don't really have a containers/ folder any more for redux-store connected components.

What do you think? We can also keep it where it is for now reconsider in future prs.

Moving to /pages/admin

ChristopherChudzicki · 2022-08-02T20:23:41Z

frontends/infinite-corridor/src/components/FieldAvatar.test.tsx

+  it("Displays a small avatar image for the field", async () => {
+    const field = makeField()
+    render(<FieldAvatar field={field} imageSize="small" />)
+    const img = screen.getByAltText(`Channel avatar for ${field.title}`)


Suggestion: I think we should get rid of the alt text, and replace this with getByRole("img").

I am by no means an accessibility expert, but my understanding is that alt text is better left blank if it does not add meaningful context. Like on the field page, a screenreader would currently read, I believe

"Channel avatar for physics. Physics".

Better (I think?) just to say "Physics".

Related: https://silktide.com/blog/is-alt-text-making-your-site-less-accessible/

Actually: Probably best to make this component accept alt as a prop. Then if we ever use the avatar as an unlabeled link, we can use pass an alt tag for it. But... we don't need that right now.

ChristopherChudzicki · 2022-08-02T21:42:38Z

frontends/infinite-corridor/src/components/forms/EditFieldAppearanceForm.test.tsx

+    fireEvent.change(titleInput, {
+      target: { value: "" }
+    })
+    fireEvent.blur(titleInput)


@mbertrand One more "by the way" comment: I usually trigger events via user:

import user from "@testing-library/userEvent" // test-utils also exports this as `{ user }` for convenience. user.click(htmlElement) user.type("blah blah blah{Enter}")

The user interface is a higher-level API that more closely resembles real user interaction. (@testing-library's general philosophy is "write tests that resemble user interaction".)

For example, user.type types the text (character by character, I think... which is still plenty fast). And user.click fires most (all?) the events that would be fired by a click: pointerdown, pointermove, pointerup, click.

Anyway, that's why I generally use those methods though I suspect it doesn't usually matter.

BTW: user.type can be slightly annoying because it's like typing...you have to clear the text first:

input.setSelectionRange(0, input.value.length) // select all the text user.type(input, 'whatever new thing you want') // typing clears the selected text

testing-library/user-event#232

cc @abeglova

Oh! I remember why user.click is nice for clicking... it does not trigger events if the element is disabled, whereas fireEvent just triggers the event no matter what.

Related...

dispatching click event executes click handler for disabled elements jsdom/jsdom#2665 _Closed because apparently this follows some spec, and browsers do not follow the spec. 🤷 _

fireEvent.click() performs click on disabled input field testing-library/dom-testing-library#92

Thanks, I noticed that user.type(input, text) throws an error if text is blank ("") - Expected key descriptor but found "" in "" - so I left that test as is but changed the others.

ChristopherChudzicki · 2022-08-03T13:18:45Z

frontends/infinite-corridor/src/components/FieldAvatar.tsx

+            alt={`Channel avatar for ${field.title}`}
+            className={`avatar-image`}


All changes look good. I saw a change to the test to not use alt text...did you want to change the alt text to "" now?

Note: alt="" is different from no alt. My understanding is that if there is no alt, screen readers tend ot read the src attribute. Which seems like a strange default to me.

related: https://mitodl.slack.com/archives/G1VJY4CCV/p1658871529668839

mbertrand added the Work in Progress label Jul 28, 2022

mbertrand force-pushed the mb/field_appearance branch 5 times, most recently from c1cde9c to 6dcf480 Compare July 28, 2022 23:02

ChristopherChudzicki reviewed Jul 29, 2022

View reviewed changes

frontends/infinite-corridor/src/libs/react-query.ts Outdated Show resolved Hide resolved

frontends/infinite-corridor/src/libs/react-query.ts Outdated Show resolved Hide resolved

mbertrand force-pushed the mb/field_appearance branch 2 times, most recently from 38a4947 to f92724f Compare July 29, 2022 14:42

mbertrand force-pushed the mb/field_appearance branch 2 times, most recently from 6034e74 to a2cbf8a Compare August 2, 2022 14:41

mbertrand added Needs Review and removed Work in Progress labels Aug 2, 2022

ChristopherChudzicki self-assigned this Aug 2, 2022

ChristopherChudzicki reviewed Aug 2, 2022

View reviewed changes

frontends/infinite-corridor/src/components/forms/EditChannelAppearanceForm.tsx Outdated Show resolved Hide resolved

ChristopherChudzicki requested changes Aug 2, 2022

View reviewed changes

ChristopherChudzicki reviewed Aug 2, 2022

View reviewed changes

ChristopherChudzicki added the Waiting on Author label Aug 2, 2022

ChristopherChudzicki approved these changes Aug 2, 2022

View reviewed changes

ChristopherChudzicki removed the Needs Review label Aug 2, 2022

ChristopherChudzicki reviewed Aug 2, 2022

View reviewed changes

mbertrand force-pushed the mb/field_appearance branch from 87fc407 to fcb6bb2 Compare August 3, 2022 13:05

ChristopherChudzicki approved these changes Aug 3, 2022

View reviewed changes

Preliminary field edit page with appearance form tab

7feba81

mbertrand force-pushed the mb/field_appearance branch from fcb6bb2 to 7feba81 Compare August 3, 2022 13:22

mbertrand merged commit 48249b3 into master Aug 3, 2022

odlbot mentioned this pull request Aug 3, 2022

Release 0.192.1 #3671

Merged

4 tasks

rhysyngsun deleted the mb/field_appearance branch March 10, 2023 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Field edit page with preliminary appearance tab #3661

Field edit page with preliminary appearance tab #3661

mbertrand commented Jul 28, 2022 •

edited

ChristopherChudzicki left a comment

codecov-commenter commented Jul 29, 2022 •

edited

ChristopherChudzicki left a comment

ChristopherChudzicki Aug 2, 2022

ChristopherChudzicki Aug 2, 2022

ChristopherChudzicki Aug 2, 2022

ChristopherChudzicki Aug 2, 2022

mbertrand Aug 3, 2022

ChristopherChudzicki Aug 2, 2022

ChristopherChudzicki Aug 2, 2022

mbertrand Aug 2, 2022

mbertrand Aug 2, 2022

ChristopherChudzicki Aug 2, 2022

mbertrand Aug 3, 2022

ChristopherChudzicki Aug 2, 2022

ChristopherChudzicki Aug 2, 2022

ChristopherChudzicki Aug 2, 2022

mbertrand Aug 3, 2022

ChristopherChudzicki Aug 3, 2022

		titleError = await screen.findByText("Title is required")
		expect(titleError).toBeInTheDocument()

	titleError = await screen.findByText("Title is required")
	expect(titleError).toBeInTheDocument()
	await screen.findByText("Title is required")

		alt={`Channel avatar for ${field.title}`}
		className={`avatar-image`}

Field edit page with preliminary appearance tab #3661

Field edit page with preliminary appearance tab #3661

Conversation

mbertrand commented Jul 28, 2022 • edited

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Screenshots (if appropriate)

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 29, 2022 • edited

Codecov Report

ChristopherChudzicki 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

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

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

mbertrand commented Jul 28, 2022 •

edited

codecov-commenter commented Jul 29, 2022 •

edited