Code Review Friday: Let's Improve your code, ask Corvid master

Ask the community
, ,
1

May 8th, 2020
Join this week code review organized by Corvid master,
Comment your code below(as per instruction) we will analyze provide you suggestion or refactored code that will be easy to read, to modify, and plenty of good practice.

We also welcome experience corvid forum members to share the knowledge and expertise.

Instructions:

*1: FULL PAGE CODE is recommended
*2: Include database structure like Collection name & Field type (if you refer them in the code)
3: Is this a live site? If so, drop a link so, we can check the code.
4: Videos or Images for explanation of what the code does is useful!
Marked * are required

Don’ts

1: Do NOT include any sensitive code (like passwords, secrect key).

// try to rewrite it as xxx if it's used inside the code
const key = "XXX-XXX";

2: Asking any questions unrelated to code review or corvid
3: Screenshot the code and not pasting the code in text format
4: Code that was not working before and asking to fix (Please create a new post in the community, This Post is dedicated to improve your current working code)

Notes

  • We do code review every friday PST timezone!

  • This post will be locked after 24 hours

2

Hi every one, I’ve some trouble to create a Frozen Header That Changes as Visitors Scroll ( https://www.youtube.com/watch?v=8iO0G_qWqRY&feature=youtu.be wix tutorial video). I’m totally new, and try to code some things since a few weeks on my website, successfully for the moment thank’s to what I can find here on the forum

So, for my animated header, I’ve created two strips, thaht I’ve attached to my header :

  • One strip is with a white back ground (this is the strip I want to appear when user scrolls down, and I chose “hidden on load”)

  • One strip with green background (the one I want to be loaded first on the website

this is my code :

export function anchor1_viewportEnter(event) {
$w(‘ #whiteStrip ').hide();
//Add your code for this event here:
}

export function anchor1_viewportLeave(event) {
$w(’ #whiteStrip ').show();
//Add your code for this event here:
}

Can’t understand what’s going wrong, as I followed exactly what the video says

thank’s a lot

3

Alright I have one that I know you guys can improve on. My family owns a small restaurant who is in light of current conditions providing curbside pickup. I created a page to display inside the restaurant when a person arrives to pickup their food. A sign in front of the parking spot tells them to text a phone number linked to Twilio, some black magic gets their information into a collection in Wix that includes Spot# Order# and Name. That collection is that displayed on the site.

Here is where things got a little complicated from a collection and coding perspective. I wanted to be able to show all of the spots in a static order regardless of wether or not someone is actually in the spot ie.
Spot 1 Order # 44658 Name - Apple
Spot 2 Order # ------ Name -
Spot 3 Order # 55467 Name - Smith
etc…

However in a repeater based on a single dataset while I could sort the spots 1-8 I can not statically sent their position. So since customers are welcome to park wherever they would like could potentially get:

Spot 1 Order # 44658 Name - Apple
Spot 3 Order # 55467 Name - Smith

So I had to create a dataset for each spot filtering based on spot number and returning only one result. That forced me to have all of my code essentially copied 8 times one for each spot and dataset. I also wanted to change the background color of the container, green for brand new, yellow for 5 minutes old and red for older than ten.

If anyone has any ideas specifically on two topics I would appreciate it. First, how can I ensure the place holder repeater and text shows up when an dataset is empty? And second, is there a way to consolidate this code down to a single set rather than 8?

Only non visible code is onClick events for each button that calls the corresponding DeleteItem for that container and onitemready to call the corresponding function.

// For full API documentation, including code examples, visit https://wix.to/94BuAAs
import wixLocation from 'wix-location';
import wixData from 'wix-data';

var greenbkg = "https://static.wixstatic.com/media/748ef1_a810775075784013bef638100f30c515~mv2.jpg";
var yellowbkg = "https://static.wixstatic.com/media/748ef1_e480defce92440d7874e533917218c59~mv2.jpg";
var redbkg = "https://static.wixstatic.com/media/748ef1_5c81bb0ab0b049ebad616c15f064827e~mv2.jpg";

$w.onReady(function () {
    setInterval(() => {
        datarefresh()
    }, 8000);
});

export function datarefresh() {
    $w('#dataset1').refresh();
    $w('#dataset2').refresh();
    $w('#dataset3').refresh();
    $w('#dataset4').refresh();
    $w('#dataset5').refresh();
    $w('#dataset6').refresh();
    $w('#dataset7').refresh();
    $w('#dataset8').refresh();
    console.log('Done refreshing Dataset');
}

////Items Ready determine bkg Color

export function itemready1($Container, itemData, index) {
 let firstItem = itemData._updatedDate
 let now = new Date();
 let dif = (now - firstItem);
 let timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = ""
 switch (true) {
 case (timeElapsed < 5):
        bkg = greenbkg;
 break;
 case (timeElapsed >= 5 && timeElapsed < 10):
        bkg = yellowbkg;
 break;
 case (timeElapsed >= 10):
        bkg = redbkg;
 break;
 default:
        bkg = "";
 break;
    }
    $w("#container1").background.src = bkg;
}

export function itemready2($Container, itemData, index) {
 let secondItem = itemData._updatedDate
 let now = new Date();
 let dif = (now - secondItem);
 let timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = ""
 switch (true) {
 case (timeElapsed < 5):
        bkg = greenbkg;
 break;
 case (timeElapsed >= 5 && timeElapsed < 10):
        bkg = yellowbkg;
 break;
 case (timeElapsed >= 10):
        bkg = redbkg;
 break;
 default:
        bkg = "";
 break;
    }
    $w("#container2").background.src = bkg;
}

export function itemready3($Container, itemData, index) {
 let thirdItem = itemData._updatedDate
 let now = new Date();
 let dif = (now - thirdItem);
 let timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = ""
 switch (true) {
 case (timeElapsed < 5):
        bkg = greenbkg;
 break;
 case (timeElapsed >= 5 && timeElapsed < 10):
        bkg = yellowbkg;
 break;
 case (timeElapsed >= 10):
        bkg = redbkg;
 break;
 default:
        bkg = "";
 break;
    }
    $w("#container3").background.src = bkg;
}

export function itemready4($Container, itemData, index) {
 let fourthItem = itemData._updatedDate
 let now = new Date();
 let dif = (now - fourthItem);
 let timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = ""
 switch (true) {
 case (timeElapsed < 5):
        bkg = greenbkg;
 break;
 case (timeElapsed >= 5 && timeElapsed < 10):
        bkg = yellowbkg;
 break;
 case (timeElapsed >= 10):
        bkg = redbkg;
 break;
 default:
        bkg = "";
 break;
    }
    $w("#container4").background.src = bkg;
}

export function itemready5($Container, itemData, index) {
 let firstItem = itemData._updatedDate
 let now = new Date();
 let dif = (now - firstItem);
 let timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = ""
 switch (true) {
 case (timeElapsed < 5):
        bkg = greenbkg;
 break;
 case (timeElapsed >= 5 && timeElapsed < 10):
        bkg = yellowbkg;
 break;
 case (timeElapsed >= 10):
        bkg = redbkg;
 break;
 default:
        bkg = "";
 break;
    }
    $w("#container5").background.src = bkg;
}

export function itemready6($Container, itemData, index) {
 let secondItem = itemData._updatedDate
 let now = new Date();
 let dif = (now - secondItem);
 let timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = ""
 switch (true) {
 case (timeElapsed < 5):
        bkg = greenbkg;
 break;
 case (timeElapsed >= 5 && timeElapsed < 10):
        bkg = yellowbkg;
 break;
 case (timeElapsed >= 10):
        bkg = redbkg;
 break;
 default:
        bkg = "";
 break;
    }
    $w("#container6").background.src = bkg;
}

export function itemready7($Container, itemData, index) {
 let thirdItem = itemData._updatedDate
 let now = new Date();
 let dif = (now - thirdItem);
 let timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = ""
 switch (true) {
 case (timeElapsed < 5):
        bkg = greenbkg;
 break;
 case (timeElapsed >= 5 && timeElapsed < 10):
        bkg = yellowbkg;
 break;
 case (timeElapsed >= 10):
        bkg = redbkg;
 break;
 default:
        bkg = "";
 break;
    }
    $w("#container7").background.src = bkg;
}

export function itemready8($Container, itemData, index) {
 let fourthItem = itemData._updatedDate
 let now = new Date();
 let dif = (now - fourthItem);
 let timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = ""
 switch (true) {
 case (timeElapsed < 5):
        bkg = greenbkg;
 break;
 case (timeElapsed >= 5 && timeElapsed < 10):
        bkg = yellowbkg;
 break;
 case (timeElapsed >= 10):
        bkg = redbkg;
 break;
 default:
        bkg = "";
 break;
    }
    $w("#container8").background.src = bkg;
}

////Button Clicks Delete Items

export function DeleteItem1(event) {
 //A wixData.query("CurbsideDisplay", options)
 let $item1 = $w.at(event.context);
    $item1("#dataset1").remove();
    $item1("#dataset1").refresh();
}

export function DeleteItem2(event) {
 //A wixData.query("CurbsideDisplay", options)
 let $item2 = $w.at(event.context);
    $item2("#dataset2").remove();
    $item2("#dataset2").refresh();
}

export function DeleteItem3(event) {
 //A wixData.query("CurbsideDisplay", options)
 let $item3 = $w.at(event.context);
    $item3("#dataset3").remove();
    $item3("#dataset3").refresh();
}

export function DeleteItem4(event) {
 //A wixData.query("CurbsideDisplay", options)
 let $item4 = $w.at(event.context);
    $item4("#dataset4").remove();
    $item4("#dataset4").refresh();
}

export function DeleteItem5(event) {
 //A wixData.query("CurbsideDisplay", options)
 let $item4 = $w.at(event.context);
    $item4("#dataset5").remove();
    $item4("#dataset5").refresh();
}

export function DeleteItem6(event) {
 //A wixData.query("CurbsideDisplay", options)
 let $item4 = $w.at(event.context);
    $item4("#dataset6").remove();
    $item4("#dataset6").refresh();
}

export function DeleteItem7(event) {
 //A wixData.query("CurbsideDisplay", options)
 let $item4 = $w.at(event.context);
    $item4("#dataset7").remove();
    $item4("#dataset7").refresh();
}

export function DeleteItem8(event) {
 //A wixData.query("CurbsideDisplay", options)
 let $item4 = $w.at(event.context);
    $item4("#dataset8").remove();
    $item4("#dataset8").refresh();
}

1050×682 39.5 KB

4

Can you also give use the dataset setting betten any two dataset1, dataset2
and the database
A screeshot of this will be good
I think we can simplify the database structure and after that we can refactor the code

5

Please use time opportunity to refactor your existing code

And to answer your question
You don’t need to use corvid to do this now
Wix support Header
https://support.wix.com/en/article/fixing-your-menu-to-the-top-of-your-site

Thank you

6

Simlify done to 60 lines :slight_smile:
Don’t repeat yourself is a good practice in coding.
Try to use a loops, proper element id (like you do for dataset1, dataset2…),
reusable functions to write less code
so, it’s easy in the eyes

you will need to rename the button delete
btnDelete1, btnDelete2…

import wixLocation from 'wix-location';
import wixData from 'wix-data';

/**
 * Assumed you are using 8 repeater
 * and each one is connected to the dataset
 * You don't need to use 8 repeater you can use a box
 * 
 * ItemReady event is it for each repeater?
 */

$w.onReady(function () {
 // REFRESH DATASETS EVERY 8 SECS
  setInterval(datarefresh, 8000);

 /**
      * Used a for loop to generate dataset1, dataset2...dataset8
      * make sure you have the right ids on the dataset, containers
      */
 for (let i = 1; i <= 8; i++) {
 // You can use box normal box elements? Why are you using a repeater?
 $w(`#dataset${i}`).onItemReady(($item, itemData) => {
 // UPDATE UI
 $item(`#container${i}`).background.src = getBackground(itemData);

 // HANDLE EVENTS
 $w(`#btnDelete${i}`).onClick(() => {
 $item(`#dataset${i}`).remove();
 $item(`#dataset${i}`).refresh();
 });
 });
 }
});

function datarefresh() {
 for (let i = 1; i <= 8; i++) $w(`#dataset${i}`).refresh();
  console.log('Done refreshing Dataset');
}

function getBackground(itemData) {

 const greenbkg = "https://static.wixstatic.com/media/748ef1_a810775075784013bef638100f30c515~mv2.jpg";
 const yellowbkg = "https://static.wixstatic.com/media/748ef1_e480defce92440d7874e533917218c59~mv2.jpg";
 const redbkg = "https://static.wixstatic.com/media/748ef1_5c81bb0ab0b049ebad616c15f064827e~mv2.jpg";

 const lastUpdateDate = itemData._updatedDate;
 const now = new Date();
 const dif = (now - lastUpdateDate);
 const timeElapsed = Math.round((dif / 1000) / 60);
 let bkg = "";

 if (timeElapsed < 5) bkg = greenbkg;
 else if (timeElapsed >= 5 && timeElapsed < 10) bkg = yellowbkg;
 else if (timeElapsed >= 10) bkg = redbkg;

 return bkg;
}
7

@salman-hammed This is exactly what I was hoping for. To answer some of your questions.

I don’t think there is a reason I was using repeater instead of a box. Are you saying I should be able to use one single large box?

Attached are screenshots of the collection filled with sample items and a couple of datasets.

2400×1599 587 KB

2400×1599 586 KB

2400×1599 546 KB

2400×1599 358 KB

8

Closing the thread
hope this was useful,
We(Corvid master) will do Code review on every Friday ,
get ready for the next code review on 15th May
Happy coding :v: