Update chat messages component to use stimulus 2 syntax #681
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roschaefer
approved these changes
Feb 6, 2021
| data-request-notification-message-template="<%= t('components.request_notification.message_template').to_json %>" | ||
| data-request-notification-id-value="<%= request.id %>" | ||
| data-request-notification-last-updated-at-value="<%= last_updated_at %>" | ||
| data-request-notification-message-template-value="<%= t('components.request_notification.message_template').to_json %>" |
There was a problem hiding this comment.
eww, JSON inside HTML attributes. Well it was there before.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #659
Recently, we upgraded to Stimulus 2.0, and updated our target syntax to get rid of deprecation warnings, but didn't make any other changes to benefit from this major version bump. The main changes in Stimulus 2.0 are related to the additions of the Values/Classes API.
Here's a link to the PR with the motivation behind these changes hotwired/stimulus#202
This PR updates the places in our code base which are based on the Data Map API, and migrates the CSS classes to the template to clean up the StimulusJS controllers, in line with the upgrade to 2.0...
Even though our project is small, and we almost exclusively use String for our values, we were able to take advantage of the explicit data type declarations for the
allowNewValue... this is also helpful for avoiding the need to useparseIntwhen declaring a Number data type.The CSS classes, I think we don't benefit as much from... It does make the Stimulus controller cleaner by moving the BEM classes out, but it adds that to the template, so not much gained, maybe it's just a personal preference. If we were to reuse the controllers for multiple templates, which would have different class names, then we would benefit more from it, but if that is the direction Stimulus - and therefore the docs - is going, I think we don't lose anything by keeping up with modern syntax.