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

AEM-3796 - Testimonials #555

Merged
merged 3 commits into from
Aug 31, 2018
Merged

Conversation

nakovskijosif
Copy link

  • Added new option to show all elements inline and disable slider.

Fixes # (AEM-3796)

Changes proposed in this pull request:

  • Added new option to show all elements inline and disable slider.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Checklist:

  • My code follows the style guidelines of this project
  • 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
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

 - Added new option to show all elements inline and disable slider.
@@ -167,6 +167,14 @@ $o-testimonials-author-position-bottom: -40px;
animation: fromRight 0.8s;
}

axa-testimonials[show-all-inline="true"] .o-testimonials__item-author {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a modifier class here instead of going onto the rendered show-all-inline attribute

Copy link
Contributor

Choose a reason for hiding this comment

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

like .o-testimonials--show-all-inline

Copy link
Author

Choose a reason for hiding this comment

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

would the below be fine?
.o-testimonials--show-all-inline .o-testimonials__item-author {
// css goes here
}
because I need this css to be applied on optional div in the child fragment.

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

 - used modifier class instead of attribute.
this.hideAllSlides();
if (this.slides.length < 2) {
// if there is only 1 slide or the option to show all slides is enabled hide the slide controls.
if (this.slides.length < 2 || this.showAllInline) {
Copy link

Choose a reason for hiding this comment

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

May it's nicer to invert the logic here and only call showControls if they are needed 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

@@ -167,6 +167,14 @@ $o-testimonials-author-position-bottom: -40px;
animation: fromRight 0.8s;
}

.o-testimonials--show-all-inline .o-testimonials__item-author {
Copy link

Choose a reason for hiding this comment

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

We are currently discussing CSS specificity and the goal of zero CSS specificity inclination #383

Copy link
Author

Choose a reason for hiding this comment

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

@LucaMele @AndyOGo
We need alternative option to display the slider in author. This is why I did this.
I can implement it in AEM without using the showAllInline tag if the way it is implemented doesn't comply with the pattern library. (zero CSS specificity inclination).

Copy link

Choose a reason for hiding this comment

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

@nakovskijosif
I would not worry about it right now, we already have higher CSS specificity code and it's an idea we think could be worth it

Copy link
Contributor

Choose a reason for hiding this comment

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

is fine as it is @nakovskijosif

Copy link

@AndyOGo AndyOGo left a comment

Choose a reason for hiding this comment

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

Fine with me

 - controls visibility logic inverted.
@nakovskijosif nakovskijosif merged commit 2bca73e into develop Aug 31, 2018
@nakovskijosif nakovskijosif deleted the bugfix/testimonials-new-option branch August 31, 2018 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants