Scales for Dummies by Nicklye26 · Pull Request #9 · rocketacademy/project1-bootcamp

Nicklye26 · 2022-10-25T12:04:40Z

No description provided.

Flashrob · 2022-10-30T15:01:24Z

package.json

    "react-scripts": "5.0.1"
  },
  "scripts": {
+    "predeploy": "npm run build",


is this inserted by github pages? Seems a bit inefficient to me :D

Flashrob · 2022-10-30T15:02:55Z

src/App.js

+function App() {
+  return (
+    <div className="App">
+      <HomePage id="Home" />


HomePage seems odd naming to me for a single-page application! Maybe something like Hero, Heading, or something along these lines would fit better, depending on what the section actually contains.

Flashrob · 2022-10-30T15:03:52Z

src/components/about-style.css

@@ -0,0 +1,21 @@
+.about-container {


I'd just name this file about.css

Flashrob · 2022-10-30T15:05:07Z

src/components/nav-links.js

+        <button className="button-1">
+          <a href="#basics-section">
+            <img src={MenuIcon1} alt="" />
+          </a>
+        </button>
+        <button className="button-1">
+          <a href="#intervals-section">
+            <img src={MenuIcon2} alt="" />
+          </a>
+        </button>
+        <button className="button-1">
+          <a href="#major-scales-section">
+            <img src={MenuIcon3} alt="" />
+          </a>
+        </button>
+        <button className="button-1">
+          <a href="#circle-of-fifths-section">
+            <img src={MenuIcon4} alt="" />
+          </a>
+        </button>


This here seems quite repetitive. We could probably use an array of objects for this, and render this via a array.map!

Suggested change

<button className="button-1">

<a href="#basics-section">

<img src={MenuIcon1} alt="" />

</a>

</button>

<button className="button-1">

<a href="#intervals-section">

<img src={MenuIcon2} alt="" />

</a>

</button>

<button className="button-1">

<a href="#major-scales-section">

<img src={MenuIcon3} alt="" />

</a>

</button>

<button className="button-1">

<a href="#circle-of-fifths-section">

<img src={MenuIcon4} alt="" />

</a>

</button>

const buttons = [{ href: "#basics-section", icon: <MenuIcon1 /> }, .....]

{buttons.map((buttonItem) =>

<button className="button-1">

<a href={buttonItem.href}>

<img src={buttonItem.icon} alt="" />

</a>

</button>

)}

Flashrob · 2022-10-30T15:07:54Z

src/homepage-welcome.js

+    <>
+      <div className="container" id="homepage">
+        <div className="wrapper">
+          <div className="home-title-1">Welcome to</div>


Instead of using classes like home-title-1 I think we could be a bit more descriptive.
In this case I would do something like home__title and home__subtitle or similarly. Can check BEM convention for CSS for more guidance https://getbem.com/naming/

Oh right, and since these are headers, it would be better to use headings instead of a div. h1,h2,h3 etc.

Flashrob · 2022-10-30T15:09:34Z

src/intervals.js

+    <>
+      <div className="padding-container" id="intervals-section">
+        <div className="container-flex">
+          <div className="title">Intervals</div>


we shouldn't use divs for everything. Divs are more like sectional dividers. For content like such, a paragraph or heading would make more sense semantically.

Flashrob · 2022-10-30T15:10:21Z

src/intervals.js

+            <p>Half tone - H</p>
+          </div>
+        </div>
+        <div className="separation-line"></div>


Could use an hr tag and style it here, I think. Or maybe use borders on the previous or following elements to create a separation line.

Flashrob · 2022-10-30T15:12:50Z

src/major-scales.js

+                </button>
+              </div>
+              <div className="flex-text">
+                <div>C D E F G A B C</div>


It would be great if you could create variables containing this data, and utilize a function like .map in order to dynamically generate the html for the whole file. Ideally we want to be as lazy as possible and try to avoid manual labour as much as possible. Try to use code to do these motivation-killing tasks for you.

Flashrob · 2022-10-30T15:13:11Z

src/major-scales.js

+                </button>
+              </div>
+              <div className="flex-text">
+                <div>Bb C D Eb F G A Bb</div>


Yup this whole file could be generated dynamically almost.

Flashrob · 2022-10-30T15:14:43Z

src/minor-scales.js

+                </button>
+              </div>
+              <div className="flex-text">
+                <div>Bb C Db Eb F Gb Ab Bb</div>


Same as with major scales, use functions to generate the HTML. See it like this, we try to let our computers work for us, not the other way around. When you write repetitive code, ask yourself if you could create this code in an easier and dynamic way. If we don't strive for that, it would be like endlessly filling in excel tables by hand.

Flashrob · 2022-10-30T15:21:48Z

src/basics.js

@@ -0,0 +1,33 @@
+import "./App.css";
+
+const Basics = () => {


components always capital letter, as well as the file name

Flashrob · 2022-10-30T15:21:58Z

src/circle-of-fifths.js

+import "./App.css";
+import ScrollToTop from "./components/scrollToTop";
+
+const CircleOfFifths = () => {


capital file name for components

Flashrob · 2022-10-30T15:22:01Z

src/components/about.js

@@ -0,0 +1,26 @@
+import "./about-style.css";
+
+const About = () => {


capital file name for components

Flashrob · 2022-10-30T15:22:05Z

src/components/nav-links.js

+import MenuIcon3 from "../assets/menu-icons-01.png";
+import MenuIcon4 from "../assets/menu-icons-04.png";
+
+const NavLinks = () => {


capital file name for components

Flashrob · 2022-10-30T15:22:49Z

src/components/scrollToTop.js

@@ -0,0 +1,17 @@
+import MenuIcon5 from "../assets/menu-icons-05.png";
+
+const ScrollToTop = () => {


This sounds like a function, not like a component. I think we could rename this. If the first word in a name is an action, it is always a function usually. Plus the file name for components need to be capital letter

Flashrob · 2022-10-30T15:22:54Z

src/homepage-welcome.js

+import "./App.css";
+import NavLinks from "./components/nav-links";
+
+const Welcome = () => {


capital file name for components

Flashrob · 2022-10-30T15:22:57Z

src/homepage.js

+import About from "./components/about";
+import Welcome from "./homepage-welcome";
+
+const HomePage = () => {


capital file name for components

Flashrob · 2022-10-30T15:23:01Z

src/intervals.js

@@ -0,0 +1,30 @@
+import "./App.css";
+
+const Intervals = () => {


capital file name for components

Flashrob · 2022-10-30T15:23:06Z

src/major-scales.js

+import "./table-style.css";
+import { Howl } from "howler";
+
+const MajorScales = () => {


capital file name for components

Flashrob · 2022-10-30T15:23:09Z

src/minor-scales.js

+import "./table-style.css";
+import { Howl } from "howler";
+
+const MinorScales = () => {


capital file name for components

Nicklye26 added 8 commits October 18, 2022 20:45

changed layout to single page application

e6d86dd

worked on CSS, next to work on buttons

a71d7f0

finished project

dddf91b

fixed certain css

4744dc6

edited the content a little bit

6feb548

edited the content a little bit

c568d8c

trying to deploy site

3359395

test publish

2954267

Flashrob reviewed Oct 30, 2022

View reviewed changes

		@@ -0,0 +1,26 @@
		import "./about-style.css";

		const About = () => {

		@@ -0,0 +1,17 @@
		import MenuIcon5 from "../assets/menu-icons-05.png";

		const ScrollToTop = () => {

		@@ -0,0 +1,30 @@
		import "./App.css";

		const Intervals = () => {

Conversation

Nicklye26 commented Oct 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flashrob Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Flashrob Nov 4, 2022 •

edited

Loading