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

dom-js #83

Merged
merged 8 commits into from
Aug 12, 2022
Merged

dom-js #83

merged 8 commits into from
Aug 12, 2022

Conversation

KonstantinOkhorzin
Copy link
Contributor

DOM

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

Copy link
Contributor

@madmaxWMFU madmaxWMFU left a comment

Choose a reason for hiding this comment

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

Great job! See some comments below.

submissions/KostiantynOkhorzin/dom/script.js Outdated Show resolved Hide resolved
submissions/KostiantynOkhorzin/dom/script.js Outdated Show resolved Hide resolved
submissions/KostiantynOkhorzin/dom/script.js Outdated Show resolved Hide resolved
submissions/KostiantynOkhorzin/dom/script.js Show resolved Hide resolved
submissions/KostiantynOkhorzin/dom/script.js Outdated Show resolved Hide resolved
submissions/KostiantynOkhorzin/dom/script.js Outdated Show resolved Hide resolved
changing a few variables, replaced map  with for, created main function showSeasonsGot.
@KonstantinOkhorzin
Copy link
Contributor Author

If I understood the comments correctly, I corrected your remarks. Please, check the changes.

Copy link
Contributor

@madmaxWMFU madmaxWMFU left a comment

Choose a reason for hiding this comment

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

Don't forget to update your local repository. Your demo not last version.

submissions/KostiantynOkhorzin/dom/script.js Outdated Show resolved Hide resolved
submissions/KostiantynOkhorzin/dom/script.js Outdated Show resolved Hide resolved
submissions/KostiantynOkhorzin/dom/script.js Outdated Show resolved Hide resolved
@KonstantinOkhorzin
Copy link
Contributor Author

Please, check the changes.

Copy link
Contributor

@madmaxWMFU madmaxWMFU left a comment

Choose a reason for hiding this comment

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

Your variable data does not change its content, so you can not pass it constantly as an argument to functions.
In your main function showSeasonsGot, leave only function calls and event handlers.

item.classList.remove(activeClassSelector);
});
btns[i].classList.add(activeClassSelector);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend to create a variable styleList which have properties with className. Because if in css some class was changed in code we must found and edit all rows with this className.

const styleList = {
   activeItemMenu: 'side-menu__btn_active',
    ...........................
}

    const toggleActiveClass = (activeBtn = 0) => {
        const btnList= document.querySelectorAll('.side-menu__btn');
        btnList.forEach(btn=> {
            if(btn.dataset.id !== activeBtn) {
                    btn.classList.remove(activeClassSelector);
            } else {
                     btn.classList.add(activeClassSelector);
            }
        });
    };

@KonstantinOkhorzin
Copy link
Contributor Author

I fixed all. Please, check the changes.

Copy link
Contributor

@madmaxWMFU madmaxWMFU left a comment

Choose a reason for hiding this comment

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

Three changes and I approve your work)

};

const renderContent = (i = 0, arr = data) => {
const {title, descr, img} = arr[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Array.filter() to select data.

item.classList.remove(styleList.itemMenu);
});
btns[i].classList.add(styleList.itemMenu);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to get something from an array you rather use the array methods. You can try the example I offered you earlier or do something different.


const toggleBurger = () => {
btnBurger.addEventListener('click', () => {
body.classList.toggle(styleList.lock);
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate this function and event listener.

const toggleBurger = () => { .... }
btnBurger.addEventListener('click', toggleBurger);

added prop id in obj data, changes function: renderBtns, renderContent, toggleActiveClass, toggleBurger
@KonstantinOkhorzin
Copy link
Contributor Author

I changes all). Please, check the changes and approve my work)

@madmaxWMFU madmaxWMFU merged commit 65f1e5f into kottans:main Aug 12, 2022
@madmaxWMFU madmaxWMFU self-assigned this Aug 23, 2022
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.

2 participants