Skip to content

Edit modal - add "Category" field to "Basic information" section#3376

Merged
rtibbles merged 9 commits intolearningequality:unstablefrom
sairina:metadata-category
May 13, 2022
Merged

Edit modal - add "Category" field to "Basic information" section#3376
rtibbles merged 9 commits intolearningequality:unstablefrom
sairina:metadata-category

Conversation

@sairina
Copy link
Copy Markdown
Contributor

@sairina sairina commented Apr 21, 2022

Summary

Description of the change(s) you made

Manual verification steps performed

  1. Checked to see if behavior of the checkboxes in the dropdown and the chips work appropriately
  2. Checked for data to be sent correctly to the backend
  3. Checked to see if tests passed

Screenshots

Chip and dropdown
Screen Shot 2022-04-25 at 9 01 30 PM
Chip overflow
Screen Shot 2022-04-25 at 9 02 03 PM

Does this introduce any tech-debt items?

The code is pretty spaghetti-ish and could use some refactoring, particularly around the behaviors of the dropdown. It should, however, currently satisfy what is needed for an MVP. We can file follow-up issues to fix. This is a continuation of this issue #3205


Reviewer guidance

How can a reviewer test these changes?

  • Any feedback on the tests would be very helpful. I'm not sure the best way to test the data being passed in (yet)
  • Manual QA will be very important! I've tried to capture as many of the interactions as I could think of below

Are there any risky areas that deserve extra testing?

It is very important to refer to the Gherkin stories and the Figma designs. There are additional scenarios that should be tested that are not described in the scenarios or in the Figma designs, but should be noted (I may also be missing some):

There are grandparents, parents, and children items

  • Check any item with no children

    • dropdown: only that item should be checked
    • chips: only that item should appear
  • Check a parent item with no children

    • dropdown: that item and the grandparent should be checked
    • chips: only that item should appear
  • Check any item with children

    • dropdown: that item and its parents should be checked
    • chips: only that item should appear
  • Check any item, and then check its child

    • dropdown: that item and its parents should be checked
    • chips: that item should be replaced with its child
  • Uncheck any child with no siblings and its parent has no siblings checked

    • dropdown: that item and all its parents should be unchecked
    • chips: remove it from chips
  • Uncheck any child with no siblings but its parent has siblings checked

    • dropdown: that item and its immediate parent should be unchecked
    • chips: remove it from chips
  • Uncheck any child with siblings and its parent has no siblings checked

    • dropdown: only that item should be unchecked
    • chips: remove that item from chips
  • Uncheck any child with siblings and its parent has siblings checked

    • dropdown: only that item should be unchecked
    • chips: remove that item from chips
  • Uncheck a parent with checked children

    • dropdown: remove that parent and its checked children
    • chips: remove the child chip

References


Contributor's Checklist

PR process:

  • If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
  • The docs label has been added if this introduces a change that needs to be updated in the user docs?

Studio-specifc:

  • All user-facing strings are translated properly
  • All UI components are LTR and RTL compliant
  • Views are organized into pages, components, and layouts directories as described in the docs

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Any new interactions have been added to the QA Sheet
  • Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@sairina sairina marked this pull request as ready for review April 26, 2022 04:37
Copy link
Copy Markdown
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, manual QA is looking good to me in terms of interactions of the chips <> checkboxes. I will do a code review tomorrow morning and also cross-reference with the Figma to make sure the interactions are aligned to the spec, but so far I have not found any unexpected edge cases 🎉 🤞

Copy link
Copy Markdown
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code read-through this morning looks good! I left a couple of comments on a couple of functions that I had to read a few times to understand. Adding comments (and for the family tree one, something about what the expect return might be) would be helpful.

I did a read through of the tests as well, but I would appreciate @rtibbles doing a closer review there since my test writing skills are very much a work in progress, and I think he would have better insight.

I know you mentioned there is one update regarding the console errors, and I am kind of assuming that is related to the perpetual "saving" state on the bottom bar. I've been able to exit the page and navigate back with state persisted, but I'm not sure if that's just in indexeddb. Looking at the networks tab, I do see /sync returning a 200 with the expected payload, so that seems good, but will again defer to @rtibbles here in case I'm missing something.

Other than that, just two notes. One re: figma specs... I remember when we were pair programming, we had the question about "Deselecting a child selects it's direct parent"

Screen Shot 2022-04-28 at 8 36 08 AM

I'm not 100% sure what this means after looking at the Figma. I think when we were pairing, we decided it meant "unselect all" which is what is happening, but I'm not sure that we were right... I think you should check with @jtamiace just to confirm.

The other small bug which I happened upon by chance is that the dropdown menus seems to be traveling around when scrolling on the page, not in the menu 😄

traveling-menu

It actually seems to be happening for all of the menus on the page, so I'm not sure this is something you did.... maybe we just never noticed before! I don't see layout changes in this PR that I think would affect this, and if you can't think of anything that you've changed in this general edit modal work that might have caused a regression, I will just open a separate issue. (I don't think this is a blocker for this, but it might be good to fix before we release the edit modal changes overall!)

@sairina
Copy link
Copy Markdown
Contributor Author

sairina commented Apr 28, 2022

The other small bug which I happened upon by chance is that the dropdown menus seems to be traveling around when scrolling on the page, not in the menu 😄

traveling-menu

It actually seems to be happening for all of the menus on the page, so I'm not sure this is something you did.... maybe we just never noticed before! I don't see layout changes in this PR that I think would affect this, and if you can't think of anything that you've changed in this general edit modal work that might have caused a regression, I will just open a separate issue. (I don't think this is a blocker for this, but it might be good to fix before we release the edit modal changes overall!)

So, this is a long-standing issue in Vuetify (see here). I can fix it for all the fields where that is a problem in the edit modal. I will open a new PR to fix those in the existing fields, and I'll fix it on my own PRs for the new fields in the edit modal. Thanks for following up with this!

@sairina sairina mentioned this pull request Apr 28, 2022
24 tasks
@sairina sairina force-pushed the metadata-category branch from 78573b1 to 88cbe70 Compare April 28, 2022 22:13
@sairina sairina requested a review from marcellamaki May 9, 2022 23:37
@sairina sairina force-pushed the metadata-category branch from 52be3b0 to 08150c2 Compare May 9, 2022 23:48
@sairina
Copy link
Copy Markdown
Contributor Author

sairina commented May 10, 2022

Note, I made a new commit that is related to dropdown menus being fixed in position while scrolling for dropdown menus that are not related to the category field (there is a new issue that explains why I didn't tackle category at this moment #3381 ).

The fix is needed because there is a console warning that occurs (don't think it was an issue in the original PR) that says: [Vuetify] Unable to locate target ... . I originally used the attach prop to the ref for each of the components, but it seems that there's a difference between the DOM and the shadow DOM, so I ended up attaching to an id instead.

I just added this to this PR - but I suppose I can cherry-pick this onto another branch and just add that as a separate PR. lmk

Copy link
Copy Markdown
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the dockstring. I think all of the feedback that I gave was addressed, and manual QA checks out. Makes sense to me to do the dropdowns separately, since they're not really related...just something I encountered. If @rtibbles doesn't have any additional feedback, I will ✅

@rtibbles rtibbles dismissed marcellamaki’s stale review May 13, 2022 14:55

Feedback addressed.

@rtibbles rtibbles merged commit a990119 into learningequality:unstable May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants