Skip to content

Refactor comments: get rid of react#6498

Merged
tramuntanal merged 72 commits intodecidim:developfrom
mainio:refactor/comments-remove-react
Oct 16, 2020
Merged

Refactor comments: get rid of react#6498
tramuntanal merged 72 commits intodecidim:developfrom
mainio:refactor/comments-remove-react

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented Sep 7, 2020 •
edited
Loading

🎩 What? Why?

The current comments component is an overly complex separate component in Decidim built currently in React which is extremely difficult to customize due to having to "fork" the whole React application away from Decidim core. It currently feels like a completely separate system inside the Decidim framework.

This PR refactors the comments component to work through cells and vanilla JS as developers would expect when working with the framework.

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG upgrade notes, if required
  • Finalize the PR
    • Create dynamic functionality for adding a new comment
    • Create dynamic functionality for replying to an existing comment
    • Create dynamic functionality for voting comments up/down
    • Implement dynamically display of a new comment after a successful backend response
    • Implement "comment as" dropdown when the user belongs to a group
    • Implement periodic polling for new comments similar as in the react component
    • Implement dynamic re-ordering of comments
    • Implement comment flagging
  • Add tests
    • Modify existing tests if needed after the refactor is completed
    • Add tests for comments cells
    • Add tests for comments controllers (comments and comment votes)
    • Add tests for the vanilla JS controlling dynamic functionality with the comments

@ahukkanen ahukkanen mentioned this pull request Sep 7, 2020
Closed
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Sep 7, 2020

Oh my <3

You'vee got my full support here @ahukkanen, thanks for taking care of that! 😍

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Sep 8, 2020

Awesome! 🚀

@ahukkanen
Updates to the comments cells
8630403 
- Fix display of comment replies and the conversation title
- Show reply only when able to reply
- Add blocked comments messages
@ahukkanen
Fix create comment action
cd55421
@ahukkanen
Implement comment form dynamic actions
a817e4c 
- Add functionality to the front-end comment alignment buttons
- Implement enabling/disabling the send comment button depending
  on the comment field content
@ahukkanen
Mount the comments engine
82e8169
@ahukkanen
Updates with the cells and dynamic functionality
2deb147 
- Implement add comment and reply to comment
- Fix problems with the cells in different situations and the
  single comment view
@mrcasals mrcasals mentioned this pull request Sep 16, 2020
Closed
@ahukkanen
Copy link
Copy Markdown
Contributor Author

ahukkanen commented Oct 14, 2020

@tramuntanal

Just the comments counter does not refresh when adding a comment, but I think it is the same behavior that we currently have.

I have added dynamic updates to the comments counter when comment or reply is added.

Right now there are also some conflicts to be resolved against develop.

I have merged with develop.

I've left just a couple of suggestions for test names.
[...] has left two more comments.

I have gone through these.

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Oct 15, 2020

thanks for the fast feedback @ahukkanen
But beware! It seems you did a bad merge, there are 603 changed files and a lot of contents from previous version in develop (we're now developing for 0.24, you have contents from 0.23), lots of conflicts with locale files, the changelog has also upgrade notes from 0.23, etc...
Can you check it please?

@ahukkanen
Copy link
Copy Markdown
Contributor Author

ahukkanen commented Oct 15, 2020

@tramuntanal I don't quite understand? I did git merge develop and then re-run npm build:prod. How's that bad merge (apart from the CHANGELOG which is a known issue caused by the squash commits).

I don't see any conflicts with locale files. The only changes I've done with locale are with the comments en locale file which are relevant for this PR. I see changes in 116 files which are correct.

I have fixed the conflict with CHANGELOG caused by the squash commits, that's the only issue I saw.

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Oct 15, 2020

I don't see any conflicts with locale files. (...) I see changes in 116 files which are correct.

Giving a quick look I agree, I see only relevant files for this change (mostly changes in decidim-comments)

imatge

😍 😍 😍 😍

@ahukkanen
Copy link
Copy Markdown
Contributor Author

ahukkanen commented Oct 15, 2020

@andreslucena

imatge

A bit less than half of the added lines are tests. 😉

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented Oct 16, 2020

Today you are right @ahukkanen , yesterday I was seeing "93 / 603 files viewed" and there were that much of files many corresponding to locales... It is very strange, maybe GitHub compared to another branch by error... I don't know..

Anyway, today it everything appears ok!
I go for the review

tramuntanal
tramuntanal approved these changes Oct 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

Very good job again Antti!

@tramuntanal tramuntanal merged commit b6b4f81 into decidim:develop Oct 16, 2020
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Oct 16, 2020

OH MY ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️

congratulations and thank you @ahukkanen for your work! <3

@ahukkanen ahukkanen mentioned this pull request Oct 24, 2020
Merged
12 tasks
@ahukkanen ahukkanen deleted the refactor/comments-remove-react branch November 15, 2020 19:01
@oliverbarnes oliverbarnes mentioned this pull request Dec 2, 2020
Merged
@andreslucena andreslucena mentioned this pull request Dec 17, 2020
Closed
1 task
@carlobeltrame carlobeltrame mentioned this pull request Mar 26, 2021
Closed
@ahukkanen ahukkanen mentioned this pull request May 7, 2021
Merged
12 tasks
@carolromero carolromero mentioned this pull request Feb 28, 2023
Closed
@ahukkanen ahukkanen mentioned this pull request Jun 5, 2023
Merged
38 tasks
This was referenced Feb 19, 2024
Merged
Merged
@alecslupu alecslupu mentioned this pull request Nov 14, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tramuntanal tramuntanal tramuntanal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tramuntanal tramuntanal

Labels

in-review module: comments module: core target: developer-experience

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ahukkanen @mrcasals @andreslucena @tramuntanal