fix: handle removing first item from Contact and links by thisislawatts · Pull Request #2128 · ONEARMY/community-platform

thisislawatts · 2023-02-26T12:19:47Z

PR Checklist

- Commit messages are descriptive, it will be used in our Release Notes

PR Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Developer experience (improves developer workflows for contributing to the project)

Description

Fixes an issue that occurs when attempting to remove the first list item from Contacts and Links.
Seems to be coming from: https://github.com/ONEARMY/community-platform/blob/master/src/pages/Settings/content/formSections/Fields/Link.field.tsx#L136:L136

Related chatter:

Git Issues

Closes issue reported:
https://discord.com/channels/586676777334865928/1079116300133072897/1079116300133072897

cypress · 2023-02-26T12:31:19Z

Passing run #3047 ↗︎

0	73	3	0	0

Details:

fix: remove disabled attr
Project: onearmy-community-platform	Commit: `162bebeb3b`
Status: Passed	Duration: 03:33 💡
Started: Mar 19, 2023 9:08 PM	Ended: Mar 19, 2023 9:11 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

thisislawatts · 2023-02-26T13:02:17Z

Working on a test to capture this fix. Think it will need to be at the e2e level as we don't have much testing infrastructure at a lower level around these pages.

davehakkens · 2023-03-11T18:41:50Z

I get this popup when removing the link.
Might be due to local testing?

And then this one

And the link doest get deleted..
But i'll try preview

github-actions · 2023-03-11T18:45:22Z

Visit the preview URL for this PR (updated for commit 162bebe):

https://onearmy-next--pr2128-fix-removing-first-i-qwt8q33h.web.app

_{(expires Fri, 21 Apr 2023 21:16:42 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

davehakkens · 2023-03-11T18:59:04Z

Same behaviour on Preview.
But thinking about this.. Filling in a contact detail is mandatory at the moment.
Why would there even be an option to delete it? -It can only be replaced by another link..
🤔

thisislawatts · 2023-03-12T19:18:03Z

Thanks for taking a look @davehakkens and catching that debugging alert. I have added an automated linter to catch those in future. I will play about with this a bit more locally, it makes sense that if you add a few more links you should be able to delete the first item. Although I understand that we also expect there to be one item entered.

thisislawatts · 2023-03-13T19:12:23Z

@davehakkens, updated so that the Delete button is only available if there are multiple fields available.

There is only a single field available:

There are multiple links added so any one can be removed. Although it is not possible to safe a profile using an empty value

davehakkens · 2023-03-14T06:51:29Z

There are multiple links added so any one can be removed. Although it is not possible to safe a profile using an empty value

Nice solution @thisislawatts. However the first/top url doest get removed here when i click delete button. Delete works for all the other ones but not the first

thisislawatts · 2023-03-14T19:54:42Z

Thanks for taking a look @davehakkens, I imagine that is why the tests are failing. Will circle back around to this tomorrow evening.

davehakkens · 2023-03-20T17:16:42Z

Is this ready to test again @thisislawatts ?

thisislawatts · 2023-03-20T17:18:41Z

Please do

davehakkens · 2023-03-20T17:31:26Z

works perfect 👌

AlfonsoGhislieri

Looks good @thisislawatts, great idea to add uuid as a key for the links!

Thought: Seeing as we are using a modal for the profile link deletion, as seen here, it could be nice to create a reusable modal.

I've noticed there are a few of these delete modals in the code base which would benefit from this as well (eg in HowtoStep.form and OpeningHoursPicker.field. This would help make sure that any change to the modal design can be applied easily across all components. Moreover, we could also then use it for the deletion of comments as well maybe?

I don't think this should this should fall into the scope of this bug fix, but could be something to as a potential issue if you agree?

AlfonsoGhislieri · 2023-03-21T10:09:49Z

src/pages/Howto/Content/Howto/HowToComments/HowToComments.tsx

  }

  async function handleDelete(_id: string) {
+    // eslint-disable-next-line no-alert


Question: are we planning on making these into custom modals down the line?

AlfonsoGhislieri · 2023-03-21T10:10:03Z

src/pages/Research/Content/ResearchComments/ResearchComments.tsx

+    // eslint-disable-next-line no-alert
    const confirmation = window.confirm(
      'Are you sure you want to delete this comment?',
    )


Same as above

src/pages/Settings/SettingsPage.tsx

thisislawatts · 2023-03-21T17:40:13Z

Thought: Seeing as we are using a modal for the profile link deletion, as seen here, it could be nice to create a reusable modal.

This is a reusable Modal element from oa-components, is there anything about the current implementation that could be improved to make it easier to reuse?

I've noticed there are a few of these delete modals in the code base which would benefit from this as well (eg in HowtoStep.form and OpeningHoursPicker.field. This would help make sure that any change to the modal design can be applied easily across all components. Moreover, we could also then use it for the deletion of comments as well maybe?

I don't think this should this should fall into the scope of this bug fix, but could be something to as a potential issue if you agree?

Definitely worth raising something like Consistent use of alert/modal as a separate issue, these are not consistent across the site and I think it would be great to make this experience a bit smoother. Specifically on this page I do not think an alert/modal is necessary as the removal action is not immediately destructive. The user needs to click 'Save' before the change will be persisted.

AlfonsoGhislieri · 2023-03-21T18:16:48Z

This is a reusable Modal element from oa-components, is there anything about the current implementation that could be improved to make it easier to reuse?

Ah I didn't realize it was a custom component, my bad, I assumed it was something like react-modal. Nonetheless, my thinking was that the buttons and styling could be extracted into a 'delete' modal component, and just have to pass the desired text down as a prop. Here and here are where I saw the same modal with all the same code inside of each.

The state of the modal could then be stored in something like context or redux, not sure if either is used for the project. This would avoid having to have all the logic for opening and closing the modals inside of each component.

Maybe this is just overcomplicating something that is fine, and most likely not very much a priority, but it just stuck out to me as not being very DRY.

Definitely worth raising something like Consistent use of alert/modal as a separate issue, these are not consistent across the site and I think it would be great to make this experience a bit smoother. Specifically on this page I do not think an alert/modal is necessary as the removal action is not immediately destructive. The user needs to click 'Save' before the change will be persisted.

Yes definitely, as I was going through the site at times it felt odd to use the modal and then later on have the window popout instead. I'll look into raising a issue relating to more consistent use of modals!

onearmy-bot · 2023-03-22T21:10:51Z

🎉 This PR is included in version 1.41.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

thisislawatts marked this pull request as ready for review February 26, 2023 16:01

thisislawatts requested a review from a team as a code owner February 26, 2023 16:01

davehakkens added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Mar 11, 2023

thisislawatts force-pushed the fix/removing-first-item-of-array branch from d3822ad to f7d29ee Compare March 12, 2023 16:33

github-actions bot added Mod: HowTo 📰 Research 🧪 labels Mar 12, 2023

thisislawatts added 5 commits March 19, 2023 21:58

fix: handle removing first item from Contact and links

107946a

test: add e2e coverage for editing contact and links item

031559b

chore: add linting to catch alerts

cbb12d8

fix: remove delete button from ProfileLinkField when it is the only item

8a22e93

fix: remove disabled attr

162bebe

thisislawatts force-pushed the fix/removing-first-item-of-array branch from a068f7c to 162bebe Compare March 19, 2023 20:59

davehakkens added the Stage: 🖼️ Design Approved Tagged when design is tested and approved label Mar 20, 2023

AlfonsoGhislieri approved these changes Mar 21, 2023

View reviewed changes

davehakkens approved these changes Mar 21, 2023

View reviewed changes

davehakkens merged commit 49bd749 into master Mar 21, 2023

davehakkens deleted the fix/removing-first-item-of-array branch March 21, 2023 19:45

cypress bot mentioned this pull request Mar 21, 2023

🤖 Release PR #2151

Merged

onearmy-bot added the released label Mar 22, 2023

Uh oh!

Conversation

thisislawatts commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Git Issues

Uh oh!

cypress bot commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Passing run #3047 ↗︎

Uh oh!

thisislawatts commented Feb 26, 2023

Uh oh!

davehakkens commented Mar 11, 2023

Uh oh!

github-actions bot commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davehakkens commented Mar 11, 2023

Uh oh!

thisislawatts commented Mar 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisislawatts commented Mar 13, 2023

Uh oh!

davehakkens commented Mar 14, 2023

Uh oh!

thisislawatts commented Mar 14, 2023

Uh oh!

davehakkens commented Mar 20, 2023

Uh oh!

thisislawatts commented Mar 20, 2023

Uh oh!

davehakkens commented Mar 20, 2023

Uh oh!

AlfonsoGhislieri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlfonsoGhislieri Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

AlfonsoGhislieri Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thisislawatts commented Mar 21, 2023

Uh oh!

AlfonsoGhislieri commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onearmy-bot commented Mar 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thisislawatts commented Feb 26, 2023 •

edited

Loading

cypress bot commented Feb 26, 2023 •

edited

Loading

github-actions bot commented Mar 11, 2023 •

edited

Loading

thisislawatts commented Mar 12, 2023 •

edited

Loading

AlfonsoGhislieri left a comment •

edited

Loading

AlfonsoGhislieri commented Mar 21, 2023 •

edited

Loading