Skip to content

Tags & mentions#2684

Merged
josepjaume merged 33 commits intomasterfrom
design/tags-mentions
Mar 22, 2018
Merged

Tags & mentions#2684
josepjaume merged 33 commits intomasterfrom
design/tags-mentions

Conversation

@Crashillo
Copy link
Copy Markdown
Contributor

@Crashillo Crashillo commented Feb 8, 2018

🎩 What? Why?

Add support for tags and mentions. It expects to work as components out-of-the-box.
Currently is appended to the filters partial.

  • Tags
  • Mentions

📌 Related Issues

🔔 Considerations

📷 Screenshots (optional)

tags
mentions

@Crashillo Crashillo self-assigned this Feb 8, 2018
$(() => {
const $mentionContainer = $('.js-mentions-container');

const sources = [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we have this static data here? Isn't this used only as an example? If so, it should live only inside the design app (under app/assets).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, no, it's yet in progress. That's an example, you may ignore it. I'll provide further info when release. 😀
Nevertheless, input_tags.js is ready. You could check this out. I've tried to pay attention in how you add vendor & javascripts to the app, because they're gonna straight to the core.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, sorry! I was in a review spree and didn't see this was actually in progress. Ignore me! And good job!

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 9, 2018

Codecov Report

Merging #2684 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2684   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files        1716     1716           
  Lines       40967    40967           
=======================================
  Hits        40393    40393           
  Misses        574      574

@Crashillo
Copy link
Copy Markdown
Contributor Author

This PR is ready to be reviewed. codeclimate complains about the TODOs but they will still present meanwhile we've not solved the integration.
Please @decidim/lot-core, review the comments I send to see how to proceed on.


$(() => {
const $mentionContainer = $('.js-mentions');
const nodatafound = 'No results.'; // TODO: this string must be i18n
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This string is hardcoded. How can be get it from locales?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe from a data attribute?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it done anywhere else? You mean something like: data-no-results="<%= t('no-results') %>" ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that's the idea. Don't know if it's used elsewhere (I think we do use it, but can't think of a specific file that does), but yes, that's how it should work!

Then from the JS you get the value of this attribute 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, one problem less. :D

$(() => {
const $mentionContainer = $('.js-mentions');
const nodatafound = 'No results.'; // TODO: this string must be i18n
const sources = []; // TODO: Object with remote data. See Tribute.js DOCS
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need to get an array object-like to make it work. Below there is an example. I strongly recommend to read the tribute.js docs, as the remote data will come from async function, I presume, am I right?

@Crashillo
Copy link
Copy Markdown
Contributor Author

Crashillo commented Mar 6, 2018

I have included in the pattern-library the mentions feature within quill.js. And I fixed the styles properly. Let me know how do you see it @isaacmg410 , whether resume this issue or keep it fridge 'til cells come.

@isaacmg410 isaacmg410 mentioned this pull request Mar 12, 2018
6 tasks
@xabier
Copy link
Copy Markdown

xabier commented Mar 20, 2018

hi, this seems to be frozen for a couple of weeks, @agustibr can you update on this?

@agustibr
Copy link
Copy Markdown
Contributor

agustibr commented Mar 20, 2018

@xabier there are two PR's that introduce cells to decidim, PR #3022 (meeting_card) is in-review and ready to merge, once is merged I'll move forward with #2897 (proposal_card).

@xabier
Copy link
Copy Markdown

xabier commented Mar 20, 2018

@agustibr thanks for the update

@agustibr
Copy link
Copy Markdown
Contributor

@xabier PR #3022 (meeting_card) is merged! So ViewModel / Cells are available in decidim-core and decidim-meetings

@Crashillo Crashillo removed their assignment Mar 21, 2018
@ghost ghost assigned Crashillo Mar 21, 2018
@isaacmg410
Copy link
Copy Markdown
Contributor

@decidim/product After discussing it offline with @Crashillo if this PR have all checks passed with no errors, and cells are in master, I will start refactoring the hashtags issue.

@decidim/lot-core do you agree that you start a new branch from this branch? Or do you want to accept this PR and merge with master, and then I create a new branch based on master?

@xabier
Copy link
Copy Markdown

xabier commented Mar 21, 2018

@decidim/product After discussing it offline with @Crashillo if this PR have all checks passed with no errors, and cells are in master, I will start refactoring the hashtags issue.

ok!

@mrcasals
Copy link
Copy Markdown
Contributor

@isaacmg410 I think this can be merged as is, to keep PRs simpler.

Simple remember to update decidim-core/app/assets/javascripts/decidim/input_mentions.js.es6 to fetch the correct values of the sources variable.

@mrcasals
Copy link
Copy Markdown
Contributor

@isaacmg410 the JS lint fails, can you check it though please?

@isaacmg410
Copy link
Copy Markdown
Contributor

@mrcasals do this then!
@Crashillo, can you solve the test of comments, please?
@decidim/lot-core Can you notify when it will be merged?

@josepjaume
Copy link
Copy Markdown
Contributor

@isaacmg410 merging this - looking forward to seeing it implemented! 🚀

@josepjaume josepjaume merged commit 4aa287d into master Mar 22, 2018
@ghost ghost removed the status: WIP label Mar 22, 2018
@josepjaume josepjaume deleted the design/tags-mentions branch March 22, 2018 14:19
@josepjaume
Copy link
Copy Markdown
Contributor

Nice work @Crashillo !

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.

8 participants