Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for emoji reactions to user posts in frontend access dev #31

Merged
merged 5 commits into from
Oct 11, 2024

Conversation

VeronicaPim
Copy link

@VeronicaPim VeronicaPim commented Oct 10, 2024

Summary of Changes, Pull Request: Add Emoji Reactions to User Posts

This pull request attempts to implement emoji reactions for user posts in the NodeBB Bluesleep project. The goal is to allow users to select and react to posts using a predefined set of emojis, displayed in an intuitive and responsive UI. Despite passing tests, several challenges and significant issues were encountered during the implementation, which are detailed below.


Context
As part of the feature request outlined in Issue #18, the task was to make emoji reactions available for posts. The acceptance criteria specified that emojis should be presented in an intuitive, functional, and responsive manner within the NodeBB user interface (UI).

Commits Overview

  1. Initial Attempts - Adding Emoji Reactions:

    • Commit: "trying to update emoji access"
    • Changes: I made initial modifications to various .tpl files, attempting to integrate emoji reactions within the post interface.
    • Result: This resulted in over 43,000 file changes due to large-scale alterations in node_modules. Most of these changes were unintended, affecting dependencies rather than feature logic, which indicated that something was fundamentally wrong with this approach.
  2. Fixes - Reverting and Troubleshooting:

    • Commit: "fixing failures"
    • Changes: After realizing the large number of unintended changes, I reverted modifications to the affected .tpl files and attempted to fix the system. This involved restoring the add-reaction.tpl to its original state, which is controlled by NodeBB’s plugin system.
    • Result: While this cleared the 44,000+ errors and restored the build to a passing state, the emoji reactions feature was still not functioning as intended.
  3. Subsequent Fixes and Adjustments:

    • Commit: "fixed previous commit"
    • Changes: I further adjusted the code to simplify and minimize changes. At this point, I restored the core files, cleaned up the project, and attempted more focused changes in add-reaction.tpl.
    • Result: Tests started passing, but the functionality of emoji reactions was still not working correctly.

Challenges Encountered

  1. Editing .tpl Files:

    • My initial edits to the reactions.tpl and add-reaction.tpl files led to significant failures (44,000+ errors), primarily because these files were tightly coupled with NodeBB’s plugin system. After reviewing the system, it became clear that the emoji reactions implementation was more complex than anticipated, possibly requiring plugin customization rather than direct .tpl file changes.
  2. Unintended Dependency Changes:

    • A major issue was the alteration of node_modules during the process. This led to thousands of file changes that were unrelated to the feature implementation and bloated the PR. I reverted these changes, but they caused delays in understanding the root issue.
  3. Test Suite Passing Without Full Functionality:

    • The current test suite passes, but it appears that the tests are not comprehensive enough to detect the issues with the emoji reactions feature. This discrepancy suggests that more specific tests (unit and integration) are needed to verify emoji functionality.

Current Status

  • Build Status: The tests pass successfully, and the system builds without errors.
  • Feature Status: Despite the passing tests, the emoji reactions feature remains incomplete. The system is not yet rendering emoji reactions in the post UI as expected.

Next Steps

  1. Manual Testing and Further Debugging:
    Although the tests are passing, I recommend performing manual testing to confirm that the emoji reactions feature is indeed functional (as I encountered failures that the tests didn’t catch).

  2. Investigate Plugin-Based Implementation:
    Since NodeBB heavily relies on its plugin architecture, it may be more appropriate to implement emoji reactions through a plugin rather than modifying core .tpl files. This will ensure that changes are correctly integrated and avoid the unintended consequences of directly editing template files.

  3. Expand Test Coverage:
    The current test suite does not appear to cover this specific functionality comprehensively. I propose adding integration tests that specifically target emoji reactions, ensuring that the feature works as expected and that any future regressions can be caught.


Conclusion
This PR reflects significant effort to implement emoji reactions for posts. Despite encountering numerous setbacks (particularly involving template edits and dependency issues), I have reverted the unintended changes and stabilized the build. The emoji feature is not yet functional, and I welcome feedback on potential directions for resolving the remaining issues. Specifically, I am considering a plugin-based solution and would appreciate guidance on whether this is the recommended path.


To Reviewers

  • Please note that while this PR passes all tests, the emoji reactions feature is still not fully functional. The tests may need to be expanded to cover this feature.
  • I have documented the challenges and efforts extensively to illustrate the complexity of this task.
  • Any advice on whether a plugin-based approach would be more appropriate is appreciated.

Testing Instructions
To reproduce the current state:

  1. Checkout the emoji-reactions-feature branch.
  2. Run the build and execute the test suite (npm run test), which should pass.
  3. Manually navigate to a post within the NodeBB interface and attempt to interact with the emoji reactions (which are currently not functioning as expected).

Copy link

@eunseokk eunseokk left a comment

Choose a reason for hiding this comment

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

This looks good! It seems like its passing all the tests and the feature is partially done!

@VeronicaPim VeronicaPim changed the title Emoji reactions feature **EDIT THIS** Attempt to add emoji reactions to user posts. Oct 11, 2024
@VeronicaPim VeronicaPim changed the title Attempt to add emoji reactions to user posts. Allow for emoji reactions to user posts in frontend access dev. Oct 11, 2024
@VeronicaPim VeronicaPim self-assigned this Oct 11, 2024
@VeronicaPim VeronicaPim changed the title Allow for emoji reactions to user posts in frontend access dev. Allow for emoji reactions to user posts in frontend access dev Oct 11, 2024
@ericlin2 ericlin2 added this to the Sprint 2 milestone Oct 11, 2024
@VeronicaPim VeronicaPim merged commit 364d5bb into f24 Oct 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants