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

Feedback #1

Open
wants to merge 5 commits into
base: feedback
Choose a base branch
from
Open

Feedback #1

wants to merge 5 commits into from

Conversation

github-classroom[bot]
Copy link
Contributor

@github-classroom github-classroom bot commented Feb 17, 2023

👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to main since the assignment started. Your teacher can see this too.

Notes for teachers

Use this PR to leave feedback. Here are some tips:

  • Click the Files changed tab to see all of the changes pushed to main since the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.
  • Click the Commits tab to see the commits pushed to main. Click a commit to see specific changes.
  • If you turned on autograding, then click the Checks tab to see the results.
  • This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
    For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @mironovisa

index.html Outdated

<script src="./js/script.js"></script>
<script
src="https://cdn.jsdelivr.net/npm/bootstrap@5.3.0-alpha1/dist/js/bootstrap.bundle.min.js"

Choose a reason for hiding this comment

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

why import the JS part of bootstrap? do you use it?

const resultsDiv = document.getElementById("resultsDiv");
const loaderP = document.getElementById("loading");

document.addEventListener("DOMContentLoaded", function (event) {

Choose a reason for hiding this comment

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

nice! consider setting deafult image in this case, instead of hiding it.

js/script.js Outdated
loaderP.style.display = "unset";
};

hideloader = () => {

Choose a reason for hiding this comment

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

camelCase naming!

js/script.js Outdated
hideloader = () => {
loaderP.style.display = "none";
};
queryAdress = () => {

Choose a reason for hiding this comment

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

fix typo


function getNameAndSymbol(name) {
let endpoint =
"https://stock-exchange-dot-full-stack-course-services.ew.r.appspot.com/api/v3/search?query=" +

Choose a reason for hiding this comment

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

use baseURL to store 'https://stock-exchange-dot-full-stack-course-services.ew.r.appspot.com/api/v3/' instead of rewriting it. DRY

js/script.js Outdated
.then((data) => {
let promises = [];
for (let i = 0; i < 10; i++) {
let clearedsymbol;

Choose a reason for hiding this comment

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

remove unsued var

pEls[i].textContent = `Change: ${data.profile.changesPercentage}%`;
pEls[i].classList.add(changesNegPoz);
if (data.profile.changesPercentage > 0) {
pEls[i].innerHTML += ` <span style="color:green">&#9650;</span>`;

Choose a reason for hiding this comment

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

don't use inline styling (style="")

js/script.js Outdated Show resolved Hide resolved
@@ -0,0 +1,99 @@
const queryParamOfThePage = new URLSearchParams(window.location.search).get(

Choose a reason for hiding this comment

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

good usage of URLSearchParams

const data = await response.json();
const prices = data.historical.map((d) => d.close);
const dates = data.historical.map((d) => d.date);
const chart = new Chart(chartDraw, {

Choose a reason for hiding this comment

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

good usage of charts package

const tickerList = document.querySelector(".ticker-list");
function MarqueeList() {
const tickerEndpoint =
"https://stock-exchange-dot-full-stack-course-services.ew.r.appspot.com/api/v3/quotes/nyse?exchange=NASDAQ";

Choose a reason for hiding this comment

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

use baseURL var (DRY)

loadData();

const tickerList = document.querySelector(".ticker-list");
function MarqueeList() {

Choose a reason for hiding this comment

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

since this function is alredy written in script.js - instead of rewriting it, use another file for the shared logic (function and variables)

@tomerp20
Copy link

Hey Andrei, well done for submitting milestones 1-4. Overall you did good work here. The website functions as expected and you have all the required features. In addition to my specific comments over your code, I recommend you to work more on the cleanliness, readability and organization of your code. In addition, the UI you created is very unique - I suggest you to do some research about creating user interface that makes the user experience comfortable and easy to use. You can start from here :
https://www.google.com/search?q=ui+ux+basic+principles&oq=UI+UX+simple+&aqs=chrome.1.69i57j0i22i30i625l8j0i10i22i30.3143j0j7&sourceid=chrome&ie=UTF-8
Good luck (:

</div>
<div class="col-4" id="searchDiv">
<h3>.search_NASDAQ_stocks:</h3>
<input type="textbox" id="searchBox" />

Choose a reason for hiding this comment

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

the input and submit button should be created inside the searchForm class

js/marquee.js Outdated
@@ -0,0 +1,24 @@
const tickerList = document.querySelector(".ticker-list");
function MarqueeList() {

Choose a reason for hiding this comment

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

where is the class?

const baseUrl =
"https://stock-exchange-dot-full-stack-course-services.ew.r.appspot.com/api/v3";

function highlightText(text, searchTerm) {

Choose a reason for hiding this comment

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

good function

@tomerp20
Copy link

Hey Andrei, well done for submitting the project.  Currently milestones 5 & 6 are missing, and since they are the main issue of this checking round this is not a good thing. I was pretty impressed from your code of milestones 1-4, hope to see more of it in the next projects...
Good luck (:

@mironovisa
Copy link

mironovisa commented Mar 13, 2023 via email

@mironovisa
Copy link

mironovisa commented Mar 14, 2023 via email

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.

None yet

2 participants