Skip to content
OverviewSolutions
6
Comments
22
Gillian Akpeki
@akpekig

All comments

  • Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    That much styling should not be controlled in JS. Use media queries and classes to toggle the menu and determine when it's shown. Then use JS to write functions toggling the ARIA attributes. ARIA will become a lot easier. Instead of counters and conditionals based on remainders, just use booleans. Much easier to toggle and you can use the state of a boolean variable as the ARIA attribute's value. Finally, selecting all of the targeted elements at the same time instead of just one and then looping through them will greatly reduce the amount of code you have to write.

    var menuDisplay = false
...
element.attr(ariaAttribute, menuDisplay)

    Resources:

    Marked as helpful
    0
  • Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    In your code, you override the form's default method on submit and simply log the values, adding or removing the error class depending on the validation result. What you need to do is make something happen when the email/form is valid, whether that's clearing all the fields and showing a success message (such as "Form submitted!") or removing the form entirely before showing the success message.

    Additionally, there are a lot of redundant files on the repository. You can prevent these from being committed by using a .gitignore files. https://www.toptal.com/developers/gitignore has an automatic generator depending on your tech stack.

    0
  • treaty1210•80
    @treaty1210Submitted over 1 year ago

    I'm always looking for more feedback and having someone else look at my work would be helpful to learning more.

    I feel like my js is pretty messy, so I feel like I could improve on that.

    Age Calculator App

    1
    Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    To improve your Javascript, give elements a unifying class, i.e. "dateInput" and use document.querySelectorAll() with a loop. That way, instead of writing the same conditional three times with a different variable, you can just write it once.

    To reduce the size of your conditionals, you can write arrays or objects for the months with a specific number of days to validate against or convert input values to strings and use regular experessions.

    Marked as helpful
    1
  • rwigley55•20
    @rwigley55Submitted over 1 year ago

    I have been stuck in tutorial hell for the last few months, so this is my very first attempt at creating something completely on my own. Feel free to be as brutally honest with my work.

    Difficulties:

    • The SVG icons in the input fields. I converted the SVG into a React component and used CSS position property to position inside the input field. This took me forever to figure out, and I'm wondering if there's an easier way to accomplish this.
    • Styling the "/ person" to drop below the "Tip Amount" and "Total" lines without pushing the dollar amounts down with it. This was extraordinarily difficult for me. I ended up making each label into a flexbox, direction: column, and wrapped the dollar amount to the next column.
    • Some of the React logic is probably not efficient. The reset button was specifically challenging to figure out.

    Best Practices:

    • React file organization. I'm sure I should organize my React components into separate folders, but I'm not sure of React component organization best practices here.
    • My App component file seems very bloated. I think I've got everything working here, but I've seen some React App component files that are much leaner than mine is. I'm wondering if mine is too bloated and some of the logic should be moved to other components?

    Tip calculator app using React, CSS flexbox

    1
    Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    Difficulties:

    • Fastest, quickest, no-installation method: import your SVG as a component directly. Create React App lets you do this.
    import { ReactComponent as DollarSign } from 'dollar-sign.svg'

    Longer method is configuring CRA to undo a lot of its beginner limitations. At that point, you might as well just not use CRA, but also it will allow you to write code specifically telling your application how to handle SVGs, whether you want them automatically imported, treated like images, etc.

    • Personally, I would use a row flexbox/inline/inline flexbox display and instead of wrapping the dollar sign, put the "Tip Amount / person" in one element and have that wrap on itself either with a width limit or line break. Less code and definitely easier to deal with.

    • (Addressing this in second section)

    Best practices:

    I don't think there's any one way to program, but React is quite opinionated. I'd recommend reading this: https://legacy.reactjs.org/docs/design-principles.html . Composition + common abstraction means that when using React, you should undertake a secondary task that's not just writing code and segregating it by relevance, but also segregating it by repetition. Buttons that all have a className and clickHandler? That's one component. Containers taking similar props? That's another. Think of each component less like a specific thing (CircleWithLionInIt, CircleWithHorse, RedSquareWithFruit) that used once or twice with little modification and more like a generic thing (Circle, Shape) that gets used repeatedly with lots of modification (Circle animal=horse, Circle animal=Lion, Shape type=red).

    • In terms of folder organisation, best practice is usually segregation by feature. So in the case of a button, Button.js and Button.module.css would go in the same folder, Button. Button, Input, Container, these would all go in another folder, components. In the end, you'll have something like this: src -> components -> Button, Input, Container.
    Marked as helpful
    0
  • Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    Refactoring feedback:

    • When the data initially loads from the API, styles are no longer applied. You have to click the dark/light mode button for styles to come back. This is worth investigating.

    • DRY (Don't Repeat Yourself) is a principle that would help with the styles. It's great you've got a light and dark mode implemented, but both files repeat a lot of code. Seeing as Sass is modular anyway, why not make a third file with all the repeated code that acts as a common module that's never removed?

    • SOLID principles would help out with the JS part. IMO, there's a lot of getting and setting that doesn't feel as modular as they should be.

    Marked as helpful
    1
  • Sidney François•110
    @sidneyfrancoisSubmitted over 1 year ago

    I think there is so much to improve, there are many redundant classes and properties. I will add more hover effects and and simple message to show that the link was copied into the clipboard.

    Pure CSS3 and Javascript Vanilla solution

    1
    Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    It's refreshing to see pure HTML, CSS, and Javascript. It's honestly not that redundant in terms of classes. Some of your desktop font sizes are a little smaller than they should be, especially the nav links. Perhaps try more standard rounded-up font sizes, such as 1rem, 1.5rem, etc.

    0
  • Gillian Akpeki•445
    @akpekigPosted over 1 year ago
    • It's mostly responsive except for when the browser width is under 900px and the advice text is a little longer than usual. Then it overflows past the divider.
    • It looks really great! The glow on the button is particularly appealing. It would be nice to see this when the button is focused on too, as well as a hidden span text to say what the button does or an aria-label.
    • I guess because you're getting each piece of advice from an API, there's a bit of a delay between clicking the button and displaying the gotten advice. This delay lets you click the button repeatedly but doesn't really do anything during that time, so it feels stuck. I'd suggest having some user feedback during this delay, whether that's disabling the button until the data is gotten or changing the symbol to a loading one.
    • Your code largely uses a advice === null conditional, when null is a falsy value. Rather than "advice === null", you can just use !advice.
    • Furthermore, the classes don't change based on this conditional. Save yourself some lines of code by putting the conditional inside a single element.
    • You import a processed CSS file into your component, but you're using Sass and React components. Why not take advantage of this amazing tech combo and import the Sass file directly?

    Overall, really close to the designs and mostly comprehensive code so great work!!

    Marked as helpful
    0
  • Felipe Rios•280
    @WolfMozart8Submitted over 1 year ago

    This is my first time using both TypeScript and React, and it was fun, but very challenging. The page shouldn't be that difficult, but I still faced some difficulties. Aside from that, I would appreciate some feedback on my React and TypeScript skills. I know some of my code can be a bit chaotic, and the organization might not be optimal. I'm not sure if I used useContext correctly. So, any feedback is welcome. :)

    SignUp page using React + typescript

    1
    Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    Functionally, your solution is perfect!

    In terms of code, I do think there's a bit of code smell as a result of not really using React or Typescript to their fullest potential. In src/components/SignUp/Info.tsx, three elements are repeated instead of interated using a map or loop function. In src/components/SignUp/Form.tsx, you allow the pages context to be potentially null instead of writing some try-catch or conditional to make sure it always exists. I understand this was likely done to get around Typescript errors, but Typescript errors aren't meant to be gotten around like some kind of bug; they're a tool to help you spot where such validation is necessary.

    I think the best use of these frameworks appears in src/components/UI/Button.tsx. It's the perfect combination of typing props and componentisation.

    Marked as helpful
    1
  • Rob Simpson•490
    @pertrai1Submitted over 1 year ago

    Still having a hard time knowing how to use multiple background images and placement of them. Any help would be greatly appreciated. Not happy with the choices I made with the CSS.

    Vanilla JS/CSS

    1
    Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    You can use multiple background images by separating them with a comma like so:

    background: url(...) ..., url(...) ...;

    You can also split up the short-hand background into background-image, background-position, etc. This will make your styles more readable. In terms of positioning, "top", "center", etc. are default values that should be enough based on the designs, but if you need something more specific, you can use units like so:

    background-position: 30% 43%, 50% 60%;

    Unrelated, but mobile designs don't seem to be implemented and so the stars for each rating wrap (at least on my device)

    0
  • Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    Use CSS state :hover like so:

    .image:hover {
transform: scale(1.5);
}
    1
  • Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    I think your code could be improved by reducing the redundancy. A lot of the elements are selected using specific classes despite having the same functions and events. To solve this, instead of:

    const elementOne = document.querySelector(".element1");
const elementTwo = document.querySelector(".element2");
...
const elementN = document.querySelector(".elementN");

    give them a unifying class name that can call them all simultaneously:

    const elements = document.querySelectorAll(".element");

    Then you can write a function that handles the conditional logic and pass it to the array of elements so you aren't writing the same/similar conditionals repeatedly:

    function handleEl(el, optionalEl) {
...
}

elements.forEach(element => {
element.addEventListener('click', () => handleEl(element))
});
    Marked as helpful
    2
  • DvoraG•100
    @DvoraGSubmitted over 1 year ago

    Is there a basic rule when to use flex, when to use grid? Which properties are necessary to set for centering containers (width, width and height)?

    Testimonials grid with grid area

    1
    Gillian Akpeki•445
    @akpekigPosted over 1 year ago

    Generally, use whichever one requires less code. Flex is my personal go-to for items of the same size in repeating rows/columns (this requires about three lines of code with flex). Grid is my go-to when items are different sizes, asymmetrical, oddly spaced, etc. Grid lets you control this a lot easier than flex, which tends to rely on margins, orders, padding, fixed sizing and breakpoints to achieve the same result.

    Depending on how you center a container, you may not need to set its size. Width is typically only necessary when using margin: 0 auto outside display flex.

    0