nausharipov commented on code in PR #17205:
URL: https://github.com/apache/beam/pull/17205#discussion_r844648342
##########
website/www/site/assets/js/quotes-slider.js:
##########
@@ -10,33 +10,99 @@
// License for the specific language governing permissions and limitations
under
// the License.
-var slider = new KeenSlider("#my-keen-slider", {
- loop: true,
- created: function (instance) {
- var dots_wrapper = document.getElementById("dots");
- var slides = document.querySelectorAll(".keen-slider__slide");
- slides.forEach(function (t, idx) {
- var dot = document.createElement("button");
- dot.classList.add("dot");
- dots_wrapper.appendChild(dot);
- dot.addEventListener("click", function () {
- instance.moveToSlide(idx);
- });
+function calcBodyWidth() {
+ return window.innerWidth && document.documentElement.clientWidth ?
+ Math.min(window.innerWidth, document.documentElement.clientWidth) :
+ window.innerWidth ||
+ document.documentElement.clientWidth ||
+ document.getElementsByTagName('body')[0].clientWidth;
+}
+
+(function () {
+
+ var CountOfSlides = {
+ min: 1,
+ renderedOnDesktop: 3,
+ }
+
+ var Selectors = {
+ desktopSlider: '.quotes-desktop.keen-slider-JS',
+ mobileSlider: '.quote-mobile.keen-slider-JS',
+ oneSlide: '.keen-slider__slide',
+ }
+
+ var classOneSlideExtra = 'wrap-slide';
+ var classOneSlide = 'keen-slider__slide';
+ var classVisible = 'visible';
+
+ var tabletWidth = 1024;
Review Comment:
the values that don't change should be constant. For example:
`const tabletWidth = 1024;`
##########
website/www/site/assets/js/quotes-slider.js:
##########
@@ -10,33 +10,99 @@
// License for the specific language governing permissions and limitations
under
// the License.
-var slider = new KeenSlider("#my-keen-slider", {
- loop: true,
- created: function (instance) {
- var dots_wrapper = document.getElementById("dots");
- var slides = document.querySelectorAll(".keen-slider__slide");
- slides.forEach(function (t, idx) {
- var dot = document.createElement("button");
- dot.classList.add("dot");
- dots_wrapper.appendChild(dot);
- dot.addEventListener("click", function () {
- instance.moveToSlide(idx);
- });
+function calcBodyWidth() {
+ return window.innerWidth && document.documentElement.clientWidth ?
+ Math.min(window.innerWidth, document.documentElement.clientWidth) :
+ window.innerWidth ||
+ document.documentElement.clientWidth ||
+ document.getElementsByTagName('body')[0].clientWidth;
+}
+
+(function () {
+
+ var CountOfSlides = {
+ min: 1,
+ renderedOnDesktop: 3,
+ }
+
+ var Selectors = {
+ desktopSlider: '.quotes-desktop.keen-slider-JS',
+ mobileSlider: '.quote-mobile.keen-slider-JS',
+ oneSlide: '.keen-slider__slide',
+ }
+
+ var classOneSlideExtra = 'wrap-slide';
+ var classOneSlide = 'keen-slider__slide';
+ var classVisible = 'visible';
+
+ var tabletWidth = 1024;
+ var bodyWidth = calcBodyWidth();
+ var isDesktopWidth = bodyWidth >= tabletWidth;
+
+ var currentSliderSelector = isDesktopWidth ? Selectors.desktopSlider :
Selectors.mobileSlider;
+ var currentSliderDOM = document.querySelector(currentSliderSelector);
+ currentSliderDOM.classList.add(classVisible);
+
+ var slidesDOM = currentSliderDOM.querySelectorAll(".keen-slider__slide");
+ var actualCountOfSlides =
currentSliderDOM.querySelectorAll(Selectors.oneSlide).length;
+
+ // create and add additional div wrappers over group of 3 cards
+ if (isDesktopWidth) {
+ var numOfExtraGroupWrappers = Math.ceil(actualCountOfSlides /
CountOfSlides.renderedOnDesktop);
+ var extraGroupWrappers = new
Array(numOfExtraGroupWrappers).fill(null).map(() => {
+ var extraGroupWrapperDOM = document.createElement('div');
+ extraGroupWrapperDOM.classList.add(classOneSlide, classOneSlideExtra);
+ return extraGroupWrapperDOM;
+ });
+ slidesDOM.forEach((oneSlideDOM, idx) => {
+ oneSlideDOM.classList.remove(classOneSlide);
+ extraGroupWrappers[Math.floor(idx /
CountOfSlides.renderedOnDesktop)].append(oneSlideDOM);
+ });
+ extraGroupWrappers.forEach((oneExtraGroupWrapper) => {
+ currentSliderDOM.append(oneExtraGroupWrapper);
});
- updateClasses(instance);
- },
- slideChanged(instance) {
- updateClasses(instance);
}
-});
-function updateClasses(instance) {
- var slide = instance.details().relativeSlide;
+ var isNeedLoop = true;
+ if (isDesktopWidth && actualCountOfSlides <=
CountOfSlides.renderedOnDesktop) {
+ isNeedLoop = false;
+ }
+ if (!isDesktopWidth && actualCountOfSlides === CountOfSlides.min) {
+ isNeedLoop = false;
+ }
- var dots = document.querySelectorAll(".dot");
- dots.forEach(function (dot, idx) {
- idx === slide
- ? dot.classList.add("dot--active")
- : dot.classList.remove("dot--active");
+ var slider = new KeenSlider(currentSliderSelector, {
Review Comment:
a warning from IDE: 'slider' is declared but its value is never read.
##########
website/www/site/assets/js/quotes-slider.js:
##########
@@ -10,33 +10,99 @@
// License for the specific language governing permissions and limitations
under
// the License.
-var slider = new KeenSlider("#my-keen-slider", {
- loop: true,
- created: function (instance) {
- var dots_wrapper = document.getElementById("dots");
- var slides = document.querySelectorAll(".keen-slider__slide");
- slides.forEach(function (t, idx) {
- var dot = document.createElement("button");
- dot.classList.add("dot");
- dots_wrapper.appendChild(dot);
- dot.addEventListener("click", function () {
- instance.moveToSlide(idx);
- });
+function calcBodyWidth() {
+ return window.innerWidth && document.documentElement.clientWidth ?
+ Math.min(window.innerWidth, document.documentElement.clientWidth) :
+ window.innerWidth ||
+ document.documentElement.clientWidth ||
+ document.getElementsByTagName('body')[0].clientWidth;
+}
+
+(function () {
+
+ var CountOfSlides = {
+ min: 1,
+ renderedOnDesktop: 3,
+ }
+
+ var Selectors = {
+ desktopSlider: '.quotes-desktop.keen-slider-JS',
+ mobileSlider: '.quote-mobile.keen-slider-JS',
+ oneSlide: '.keen-slider__slide',
+ }
+
+ var classOneSlideExtra = 'wrap-slide';
+ var classOneSlide = 'keen-slider__slide';
+ var classVisible = 'visible';
+
+ var tabletWidth = 1024;
+ var bodyWidth = calcBodyWidth();
+ var isDesktopWidth = bodyWidth >= tabletWidth;
+
+ var currentSliderSelector = isDesktopWidth ? Selectors.desktopSlider :
Selectors.mobileSlider;
+ var currentSliderDOM = document.querySelector(currentSliderSelector);
+ currentSliderDOM.classList.add(classVisible);
+
+ var slidesDOM = currentSliderDOM.querySelectorAll(".keen-slider__slide");
+ var actualCountOfSlides =
currentSliderDOM.querySelectorAll(Selectors.oneSlide).length;
+
+ // create and add additional div wrappers over group of 3 cards
+ if (isDesktopWidth) {
Review Comment:
Is it possible to move the contents of this if statement into a separate
function? It would decrease the size of the parent function and make the
comment unnecessary
##########
website/www/site/assets/js/quotes-slider.js:
##########
@@ -10,33 +10,99 @@
// License for the specific language governing permissions and limitations
under
// the License.
-var slider = new KeenSlider("#my-keen-slider", {
- loop: true,
- created: function (instance) {
- var dots_wrapper = document.getElementById("dots");
- var slides = document.querySelectorAll(".keen-slider__slide");
- slides.forEach(function (t, idx) {
- var dot = document.createElement("button");
- dot.classList.add("dot");
- dots_wrapper.appendChild(dot);
- dot.addEventListener("click", function () {
- instance.moveToSlide(idx);
- });
+function calcBodyWidth() {
+ return window.innerWidth && document.documentElement.clientWidth ?
+ Math.min(window.innerWidth, document.documentElement.clientWidth) :
+ window.innerWidth ||
+ document.documentElement.clientWidth ||
+ document.getElementsByTagName('body')[0].clientWidth;
+}
+
+(function () {
+
+ var CountOfSlides = {
+ min: 1,
+ renderedOnDesktop: 3,
+ }
+
+ var Selectors = {
+ desktopSlider: '.quotes-desktop.keen-slider-JS',
+ mobileSlider: '.quote-mobile.keen-slider-JS',
+ oneSlide: '.keen-slider__slide',
+ }
+
+ var classOneSlideExtra = 'wrap-slide';
+ var classOneSlide = 'keen-slider__slide';
+ var classVisible = 'visible';
+
+ var tabletWidth = 1024;
+ var bodyWidth = calcBodyWidth();
+ var isDesktopWidth = bodyWidth >= tabletWidth;
+
+ var currentSliderSelector = isDesktopWidth ? Selectors.desktopSlider :
Selectors.mobileSlider;
+ var currentSliderDOM = document.querySelector(currentSliderSelector);
+ currentSliderDOM.classList.add(classVisible);
+
+ var slidesDOM = currentSliderDOM.querySelectorAll(".keen-slider__slide");
+ var actualCountOfSlides =
currentSliderDOM.querySelectorAll(Selectors.oneSlide).length;
+
+ // create and add additional div wrappers over group of 3 cards
+ if (isDesktopWidth) {
+ var numOfExtraGroupWrappers = Math.ceil(actualCountOfSlides /
CountOfSlides.renderedOnDesktop);
+ var extraGroupWrappers = new
Array(numOfExtraGroupWrappers).fill(null).map(() => {
+ var extraGroupWrapperDOM = document.createElement('div');
+ extraGroupWrapperDOM.classList.add(classOneSlide, classOneSlideExtra);
+ return extraGroupWrapperDOM;
+ });
+ slidesDOM.forEach((oneSlideDOM, idx) => {
+ oneSlideDOM.classList.remove(classOneSlide);
+ extraGroupWrappers[Math.floor(idx /
CountOfSlides.renderedOnDesktop)].append(oneSlideDOM);
+ });
+ extraGroupWrappers.forEach((oneExtraGroupWrapper) => {
+ currentSliderDOM.append(oneExtraGroupWrapper);
});
- updateClasses(instance);
- },
- slideChanged(instance) {
- updateClasses(instance);
}
-});
-function updateClasses(instance) {
- var slide = instance.details().relativeSlide;
+ var isNeedLoop = true;
Review Comment:
the isNeedLoop name is grammatically incorrect. Consider renaming it to
isLoop or something more appropriate.
##########
website/www/site/assets/js/quotes-slider.js:
##########
@@ -10,33 +10,99 @@
// License for the specific language governing permissions and limitations
under
// the License.
-var slider = new KeenSlider("#my-keen-slider", {
- loop: true,
- created: function (instance) {
- var dots_wrapper = document.getElementById("dots");
- var slides = document.querySelectorAll(".keen-slider__slide");
- slides.forEach(function (t, idx) {
- var dot = document.createElement("button");
- dot.classList.add("dot");
- dots_wrapper.appendChild(dot);
- dot.addEventListener("click", function () {
- instance.moveToSlide(idx);
- });
+function calcBodyWidth() {
+ return window.innerWidth && document.documentElement.clientWidth ?
+ Math.min(window.innerWidth, document.documentElement.clientWidth) :
+ window.innerWidth ||
+ document.documentElement.clientWidth ||
+ document.getElementsByTagName('body')[0].clientWidth;
+}
+
+(function () {
+
+ var CountOfSlides = {
+ min: 1,
+ renderedOnDesktop: 3,
+ }
+
+ var Selectors = {
+ desktopSlider: '.quotes-desktop.keen-slider-JS',
+ mobileSlider: '.quote-mobile.keen-slider-JS',
+ oneSlide: '.keen-slider__slide',
+ }
+
+ var classOneSlideExtra = 'wrap-slide';
+ var classOneSlide = 'keen-slider__slide';
+ var classVisible = 'visible';
+
+ var tabletWidth = 1024;
+ var bodyWidth = calcBodyWidth();
+ var isDesktopWidth = bodyWidth >= tabletWidth;
+
+ var currentSliderSelector = isDesktopWidth ? Selectors.desktopSlider :
Selectors.mobileSlider;
+ var currentSliderDOM = document.querySelector(currentSliderSelector);
+ currentSliderDOM.classList.add(classVisible);
+
+ var slidesDOM = currentSliderDOM.querySelectorAll(".keen-slider__slide");
+ var actualCountOfSlides =
currentSliderDOM.querySelectorAll(Selectors.oneSlide).length;
+
+ // create and add additional div wrappers over group of 3 cards
+ if (isDesktopWidth) {
+ var numOfExtraGroupWrappers = Math.ceil(actualCountOfSlides /
CountOfSlides.renderedOnDesktop);
+ var extraGroupWrappers = new
Array(numOfExtraGroupWrappers).fill(null).map(() => {
+ var extraGroupWrapperDOM = document.createElement('div');
+ extraGroupWrapperDOM.classList.add(classOneSlide, classOneSlideExtra);
+ return extraGroupWrapperDOM;
+ });
+ slidesDOM.forEach((oneSlideDOM, idx) => {
+ oneSlideDOM.classList.remove(classOneSlide);
+ extraGroupWrappers[Math.floor(idx /
CountOfSlides.renderedOnDesktop)].append(oneSlideDOM);
+ });
+ extraGroupWrappers.forEach((oneExtraGroupWrapper) => {
+ currentSliderDOM.append(oneExtraGroupWrapper);
});
- updateClasses(instance);
- },
- slideChanged(instance) {
- updateClasses(instance);
}
-});
-function updateClasses(instance) {
- var slide = instance.details().relativeSlide;
+ var isNeedLoop = true;
+ if (isDesktopWidth && actualCountOfSlides <=
CountOfSlides.renderedOnDesktop) {
+ isNeedLoop = false;
+ }
+ if (!isDesktopWidth && actualCountOfSlides === CountOfSlides.min) {
+ isNeedLoop = false;
+ }
- var dots = document.querySelectorAll(".dot");
- dots.forEach(function (dot, idx) {
- idx === slide
- ? dot.classList.add("dot--active")
- : dot.classList.remove("dot--active");
+ var slider = new KeenSlider(currentSliderSelector, {
+ slidesPerView: CountOfSlides.min,
+ loop: isNeedLoop,
+ created: function (instance) {
+ if (!isNeedLoop) {
Review Comment:
can we place the below code into the if statement instead of using the blank
return?
For example:
```
if (isLoop) {
...
updateClasses(instance);
}
```
##########
website/www/site/assets/js/quotes-slider.js:
##########
@@ -10,33 +10,99 @@
// License for the specific language governing permissions and limitations
under
// the License.
-var slider = new KeenSlider("#my-keen-slider", {
- loop: true,
- created: function (instance) {
- var dots_wrapper = document.getElementById("dots");
- var slides = document.querySelectorAll(".keen-slider__slide");
- slides.forEach(function (t, idx) {
- var dot = document.createElement("button");
- dot.classList.add("dot");
- dots_wrapper.appendChild(dot);
- dot.addEventListener("click", function () {
- instance.moveToSlide(idx);
- });
+function calcBodyWidth() {
+ return window.innerWidth && document.documentElement.clientWidth ?
Review Comment:
Would using an if-else statement instead of the ternary operator improve
readability here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
