[go: up one dir, main page]

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

Refactor : change trix field show #3359

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Nevelito
Copy link

Description

  • Change trix CSS
  • change show_component for trix field
  • add new js controller for trix body
  • add new locales in every language

Fixes # (issue)
#3263

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

image
image

…troller for trix body, add new locales in every language
@github-actions github-actions bot added the Fix label Oct 24, 2024
Copy link
codeclimate bot commented Oct 24, 2024

Code Climate has analyzed commit 5775617 and detected 0 issues on this pull request.

View more on Code Climate.

@Nevelito Nevelito changed the title Refactor / change trix field show Refactor : change trix field show Oct 24, 2024
Copy link
Contributor
@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

Hey @Nevelito thank you for this contribution!

I left some comments on specific parts of the code, feel free to continue a conversation on each comment.

Additionally, I noticed that some locale files were generated under the dummy/config/locales path. Since these won't be used during development, we can remove them to keep the repo clean.

The PR is looking good overall! Let's focus on the tests and on ensuring that always_show option works as expected. We should also add a test case for always_show

app/assets/stylesheets/css/fields/trix.css Outdated Show resolved Hide resolved
Comment on lines 2 to 11
<div data-controller="trix-body">
<div class="trix-content py-2 max-w-4xl hidden" data-trix-body-target="content">
<%= sanitize @field.value.to_s %>
</div>
<div class="hidden" data-trix-body-target="moreContentButton">
<%= link_to t('avo.more_content'), 'javascript:void(0);', class: 'font-bold inline-block pt-3', data: { action: 'click->trix-body#toggleContent' } %>
</div>
<div class="hidden" data-trix-body-target="lessContentButton">
<%= link_to t('avo.less_content'), 'javascript:void(0);', class: 'font-bold inline-block pt-3', data: { action: 'click->trix-body#toggleContent' } %>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

@field.always_show seems to be ignored after these changes.

always_show is an option for the Trix field, it is documented here https://docs.avohq.io/3.0/fields/trix.html#always_show

We should ensure that setting always_show to true bypasses the entire show/hide content logic.

app/javascript/js/controllers/trix_body_controller.js Outdated Show resolved Hide resolved
Comment on lines -34 to -35
click_on "Show content"

Copy link
Contributor
@Paul-Bob Paul-Bob Oct 30, 2024

Choose a reason for hiding this comment

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

In all instances where click_on "Show content" was removed, let's update the test logic to use click_on "More content" instead.

Since "More content" only appears when there is additional content, we should add extra text to trix_post_body.

It's essential to keep each test case focused on its original purpose. Here, we only need to add more content, and the test should confirm that the entire content displays correctly after clicking "More content."

Copy link
Author

Choose a reason for hiding this comment

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

the js controller checks if the body has more then one line, so if it is not then More content button will not appear

spec/system/avo/trix_field_spec.rb Outdated Show resolved Hide resolved
@Paul-Bob
Copy link
Contributor

Just one more thing, Lint action is failing, you can check what is failing by clicking on "Details"

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants