Skip to content

chore: [M3-8262] - Cleanup eventMessages flag and legacy code#10839

Merged
abailly-akamai merged 10 commits intolinode:developfrom
abailly-akamai:M3-8262
Sep 3, 2024
Merged

chore: [M3-8262] - Cleanup eventMessages flag and legacy code#10839
abailly-akamai merged 10 commits intolinode:developfrom
abailly-akamai:M3-8262

Conversation

@abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Aug 26, 2024

Description 📝

This PR is a follow up to EventMessages v2 implementation. Now that the flag has been on in production and no major (or minor for that regard) bugs has been reported, it's time to clean up the legacy code and ordganize the code around the features EventMessages v2 introduced and implemented.

This refactor was facilited by targeting all instances of TODO eventMessagesV2 left in the codebase to this purpose.

This PR does not modify behavior or styling in any way (read shouldn't), it only reorganizes the code around the cleanup.

Changes 🔄

  • Delete eventMessagesV2 flag
  • Delete Events and NotificationCenter legacy code
  • Update NotificationCenter structure/naming conventions
  • Update tests (unit & e2e)

Preview 📷

Include a screenshot or screen recording of the change

💡 Use <video src="" /> tag when including recordings in table.

NotificationCenter Events
Screenshot 2024-08-29 at 13 12 10 Screenshot 2024-08-29 at 13 11 30

How to test 🧪

Verify that no regression happens as a result of this cleanup

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@abailly-akamai abailly-akamai self-assigned this Aug 26, 2024
@abailly-akamai abailly-akamai changed the title refactor: [M3-8262] chore: [M3-8262] - Cleanup eventMessages flag and legacy code Aug 27, 2024
</>
);
}
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and underneath isn't new code, it's just been broken down - logic is the same

borderBottom: `1px solid ${theme.bg.main}`,
position: 'relative',
},
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following our standards, broke styles and types into own files

no new code here

'community_mention',
'community_question_reply',
];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was moved from a file that was deleted (this block was added after the initial refactor)

@abailly-akamai abailly-akamai marked this pull request as ready for review August 29, 2024 20:11
@abailly-akamai abailly-akamai requested review from a team as code owners August 29, 2024 20:11
@abailly-akamai abailly-akamai requested review from bnussman-akamai, hana-akamai and jdamore-linode and removed request for a team August 29, 2024 20:11
@github-actions
Copy link

github-actions bot commented Aug 29, 2024

Coverage Report:
Base Coverage: 86.13%
Current Coverage: 86.13%

@abailly-akamai abailly-akamai requested a review from mjac0bs August 29, 2024 20:50
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Congrats on the clean up, look at that diff. 🎉 (We should remember to archive the LD flag after the release goes out too.)

Reorganizations and renamings make sense! Long, but clear and consistent, component names. 👍🏼

Didn't see any regressions after testing some events locally. CI had some trouble with a couple runners, but local runs of those tests looked good.

@abailly-akamai
Copy link
Contributor Author

Long, but clear and consistent, component names.

@mjac0bs yeah i struggled a bit with a decent convention but "Notification" in itself isn't conveying enough context. My take was that the names here makes it easier to find references for contributors in their IDE, and by compile time/minification the name length matters little.

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Aug 30, 2024
@bnussman-akamai bnussman-akamai removed the Add'tl Approval Needed Waiting on another approval! label Sep 3, 2024
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! Clean Up labels Sep 3, 2024
@abailly-akamai abailly-akamai merged commit e338cc5 into linode:develop Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved Multiple approvals and ready to merge! Clean Up

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants