Skip to content

Fix character counter with emoji picker close to maximum characters#8916

Merged
andreslucena merged 12 commits intodecidim:developfrom
eliegaboriau:fix/character_counter_emoji
Mar 14, 2022
Merged

Fix character counter with emoji picker close to maximum characters#8916
andreslucena merged 12 commits intodecidim:developfrom
eliegaboriau:fix/character_counter_emoji

Conversation

@eliegaboriau
Copy link
Copy Markdown
Contributor

@eliegaboriau eliegaboriau commented Feb 24, 2022

🎩 What? Why?

PR to make the emoji button disabled when there's not enough character left

📌 Related Issues

Testing

  • Write into a textarea until there are less than 4 characters left
  • Notice that the emoji button is disabled
  • Remove characters until there are more than 4 characters left
  • Notice that the emoji button is enabled

📋 Checklist

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

♥️ Thank you!

Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

I would like to suggest one structural change in the component so that we don't create too strong interconnection with the different JS components. I think the best way would be for each component to work as isolated as possible.

What I mean by this is that if we decide to re-write the emoji picker, we wouldn't need to do any changes in the character counter in that situation.

Based on this idea, I added one change request below.

deactivate and change color of the button
@eliegaboriau eliegaboriau requested a review from ahukkanen March 10, 2022 12:20
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Can you take a look at the comment I left above and also could you fix the eslint errors please? 🙏

linter
@eliegaboriau eliegaboriau requested a review from ahukkanen March 10, 2022 16:41
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen 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 the changes @eliegaboriau !

Getting closer, just few more change requests. 🙏

@eliegaboriau
Copy link
Copy Markdown
Contributor Author

I hope it's ok now !
Could you please launch the checks if you're fine with what i did ?

@eliegaboriau eliegaboriau requested a review from ahukkanen March 11, 2022 09:49
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

One more small thing below @eliegaboriau. 🙏

Also, could you please correct the ESLint errors. You should be able to install the ESLint extension to your code editor but if you don't know how, you can also run this command in the Decidim root to see the errors:

npm run lint

@eliegaboriau eliegaboriau requested a review from ahukkanen March 11, 2022 13:02
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the great work and the test case you added!

I also tested this locally and it works as expected.

It won't affect my approval but as a general advice I suggest not doing any unnecessary changes, such as the formatting of the import and export statements that you did. This makes your PRs much easier to review and also makes less places for merge conflicts if other people are working on the same files.

Just to take note for the future!

@ahukkanen ahukkanen added module: core type: fix PRs that implement a fix for a bug labels Mar 11, 2022
@ahukkanen ahukkanen changed the title Fix/character counter emoji Fix character counter with emoji picker close to maximum characters Mar 11, 2022
@andreslucena andreslucena merged commit 89789e8 into decidim:develop Mar 14, 2022
@andreslucena
Copy link
Copy Markdown
Member

Thanks for the PR @eliegaboriau!! Can you please backport to v0.26-stable 🙏🏽?

eliegaboriau added a commit to eliegaboriau/decidim that referenced this pull request Mar 14, 2022
@eliegaboriau
Copy link
Copy Markdown
Contributor Author

Thanks for the PR @eliegaboriau!! Can you please backport to v0.26-stable 🙏🏽?

Here it is : #9012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: core type: fix PRs that implement a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comment character count goes to negative if emojis are used after exceeding the limit

4 participants