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

Javascript-javascript3-week2 #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeli1991
Copy link

@jeli1991 jeli1991 commented May 5, 2023

No description provided.

Copy link

@dinythomas89 dinythomas89 left a comment

Choose a reason for hiding this comment

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

good work!

Copy link

@chezzoba chezzoba left a comment

Choose a reason for hiding this comment

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

Nicely done, just remember to use async / await in the future and generally wrapping the whole application logic in a single try / catch block is not the best practice since you want to actually get an error when there is one.

let exchangeRates, currencies;

async function fetchExchangeRates() {
const response = await fetch(url);
Copy link

Choose a reason for hiding this comment

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

Nicely done, but would be better with some error handling in case something goes wrong.

const option1 = document.createElement('option');
const option2 = document.createElement('option');
option1.value = currency;
option1.textContent = currency;
Copy link

Choose a reason for hiding this comment

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

To get a copy of a DOM element in JavaScript, you can use the cloneNode() method. This method creates a deep or shallow copy of the specified element, depending on the value of the argument passed to it. Here's how you can use it:

  1. Shallow copy: Pass false as an argument to cloneNode(). This will create a copy of the element without its children.
const originalElement = document.getElementById('elementId');
const shallowCopy = originalElement.cloneNode(false);
  1. Deep copy: Pass true as an argument to cloneNode(). This will create a copy of the element along with all its children and their attributes.
const originalElement = document.getElementById('elementId');
const deepCopy = originalElement.cloneNode(true);

After creating the copy, you can manipulate it or insert it into the DOM as needed. For example, you can append the copied element to another element:

const parentElement = document.getElementById('parentElementId');
parentElement.appendChild(deepCopy);

Keep in mind that the copied element will have the same id attribute as the original element, which can cause issues if you're using the id for styling or scripting purposes. To avoid this, you can change the id of the copied element:

deepCopy.id = 'newElementId';

}

fetchExchangeRates()
.then(populateDropdowns)
Copy link

Choose a reason for hiding this comment

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

Do not use .then(...).catch(...), use async / await please. This is how you could have done the same thing:

async function fetchAndUpdateExchangeRates() {
  try {
      await fetchExchangeRates();
      populateDropdowns();
      updateResult();
  } catch (error) {
      console.error(error);
      result.textContent = 'An error occurred while getting exchange rates.';
  }
}

// Call the function
fetchAndUpdateExchangeRates();

const fromSelect = document.getElementById('from');
const toSelect = document.getElementById('to');
const result = document.getElementById('result');
let exchangeRates, currencies;
Copy link

Choose a reason for hiding this comment

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

Are these global variables necessary?

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