Skip to content

Instantly share code, notes, and snippets.

@Oluwasetemi
Created November 5, 2024 14:50
Show Gist options
  • Save Oluwasetemi/6eba64caa4ae9c0d98dd67bd779e3b8e to your computer and use it in GitHub Desktop.
Save Oluwasetemi/6eba64caa4ae9c0d98dd67bd779e3b8e to your computer and use it in GitHub Desktop.

Tripleten Test Assignment

React App

1. Do the components render correctly? If not, what's wrong in the code?

Yes to some extent, the components render correctly but there are some issues in the code that need to be addressed:

The signup and signin page are not working due to the API not returning data.

  1. The ProtectedRoute component is missing the path prop in the Route component.
const ProtectedRoute = ({ component: Component, ...props }) => {
  return (
    <Route exact path={props.path}>
      {() =>
        props.loggedIn ? <Component {...props} /> : <Redirect to="./signin" />
      }
    </Route>
  );
};

This affects the routing of the application and the protected routes are not working as expected and the user is redirected to the signin page even if they are logged in.

  1. There is an unnecessary debugger statement in the Login component which should be removed for production.

  2. Add proper error handling for API calls - handleUpdateUser, handleUpdateAvatar, handleCardLike, handleCardDelete, handleAddPlaceSubmit functions need proper error handling with a .catch block to avoid unreponsive UI.(e.g. show a toast message or a modal with the error message) just like the case for the onLogin function.

  3. Use proper keys in lists - In the App component, mapping over cards using the index as the key is not recommended. Instead, use the card's unique id as the key or generate a unique key for each card or crypto.randomUUID() or uuid library.

  4. Add proper form validation for the signup and signin forms. Use a library like yup or joi or zod for form validation.

  5. Handle loading states and errors in useEffect hooks - useEffect hooks for fetching user info and cards should handle potential errors and include proper cleanup functions.

  6. Context Usage - The CurrentUserContext should be initialized with a default value like null or an empty object {} to avoid undefined errors when the context is used in components.

2. Is it possible to simplify the project infrastructure? How would you recommend reorganizing it?

The project structure can be simplified by moving components that are pages to a separate folder like pages or views. This will make it easier to distinguish between components that are pages and components that are reusable.

3. Are all routes configured correctly (including protected routes)?

  1. No, The ProtectedRoute component is missing the path prop in the Route component.
const ProtectedRoute = ({ component: Component, ...props }) => {
  return (
    <Route exact path={props.path}>
      {() =>
        props.loggedIn ? <Component {...props} /> : <Redirect to="./signin" />
      }
    </Route>
  );
};

4. Are the necessary manipulations with the JWT implemented correctly in App.js?

The JWT token should be stored in the localstorage after a successful signin.

The onSignOut function doesn't remove the JWT from localStorage. It should be:

function onSignOut() {
  localStorage.removeItem("jwt");
  setIsLoggedIn(false);
  setEmail("");
  history.push("/signin");
}

other improvements can be made to the onLogin function to include error handling when the token is not returned from the API and using the toast or modal to display the error message.

Security consideration - The JWT token should be stored in an HttpOnly cookie to prevent XSS attacks and token should be refreshed periodically to avoid unauthorized access to the application. Token should expire after a certain period of time.

Loading states with a skeleton or spineer during the token verification process can be added to improve the user experience.

Node App

1. Do all requests get the correct responses?

  1. No, the deleteCard function in the cards controller is not implemented correctly. The function should be:
const deleteCard = (req, res) => {
  const { id } = req.params;
  Card.findById(id)
    .orFail(() => {
      const error = new Error('Card not found');
      error.statusCode = 404;
      throw error;
    })
    .then((card) => Card.deleteOne(card)
      .then(() => res.send({ data: card })))
    .catch((err) => {
      if (err.kind === 'ObjectId') {
        res.status(400).send({ message: 'ID is not valid' });
      } else if (err.statusCode === 404) {
        res.status(404).send({ message: err.message });
      } else {
        res.status(500).send({ message: 'Error occurred' });
      }
    });
};

Replacing the Card.updateOne with Card.deleteOne and adding proper error handling for the delete operation.

2. Do these requests correspond to REST API principles?

Yes, Some of the basic REST principles are followed like

  1. The resource name is pluralized - cards
  2. The HTTP methods are used correctly - GET (/cards, /cards/:id), POST (/cards), DELETE (/cards/:id)
  3. Clean URLs without verbs
  4. The response structure is consistent, success response includes a data key and error response includes a message property.

but there are several improvements needed to make the API more RESTful:

  1. Fix status codes for validation errors on createCard the response code for ValidationError should be 400 Bad Request instead of 404

Another significant improvement to routes using request parameters is to validate the id parameter to ensure it is a valid ObjectId before querying the database.

if (!mongoose.Types.ObjectId.isValid(id)) {
  return res.status(400).send({ message: 'Invalid card ID' });
}
  1. Fix status code for the deleteCard function, the response code for a successful delete operation should be 204 No Content instead of 200 OK
  2. Add missing CRUD operations (PUT, PATCH)
  3. Add pagination for getCards GET /cards endpoints
const getCards = (req, res) => {
  const { page = 1, limit = 10 } = req.query;
  Card.find({})
    .limit(limit * 1)
    .skip((page - 1) * limit)
    .then((cards) => {
      res.send({
        data: cards,
        meta: {
          page: parseInt(page),
          limit: parseInt(limit)
        }
      });
    })
    .catch(() => res.status(500).send({ message: 'Error occurred' }));
};

5. Implement proper error handling

```js
const errorHandler = (err, req, res, next) => {
  const status = err.statusCode || 500;
  const message = err.message || 'Internal server error';
  res.status(status).send({ message });
};

app.use(errorHandler);

3. Is the incoming data correctly validated? The Validator.js library is used for validation. What can be removed or changed in validation if reviewers only require the URL validation for the image link?

Yes the incoming data is validated using the validator library.

const mongoose = require('mongoose');
const validator = require('validator');

const cardsSchema = new mongoose.Schema({
  name: {
    type: String,
    required: [true, 'The name field must be filled'],
    minlength: [2, 'Min length is 2'],
    maxlength: [30, 'Max length is 30'],
  },
  image: {
    type: String,
    required: [true, 'The image field must be filled'],
    validate: {
      validator: (v) => validator.isURL(v, { protocols: ['http', 'https'], require_protocol: true }),
      message: 'Must be a valid URL',
    },
  },
  createdAt: {
    type: Date,
    default: Date.now,
  },
}, { versionKey: false });

module.exports = mongoose.model('card', cardsSchema);

4. Is it possible to add or remove unintended data to database? (I am not sure what you mean by unintended data to database, this is the best way I could interpret the question.)

Yes, with mongoose we can specify add or remove data that we do not want the database to return using the select method. This can be used to exclude sensitive data like passwords or other fields that should not be returned in the response.

Card.findById(id)
    .select('name') // only return the name field

Card.findById(id)
    .select('-password') // exclude the password field

HTML & CSS

1. Is this markup semantically correct? Are there tags that are used incorrectly?

The markup is mostly semantically correct, but there are some issues:

  1. Heading element arrangement inconsistencies

The document starts with an h1 heading "What's going on in the world?" but then jumps to h3 for "Search Results" and "About author" There are inconsistent jumps between h4 and h5 headings in the preloader section Heading levels should follow a logical hierarchy (h1h2h3h4h5)

  1. Incorrect Use of Semantic Elements The section elements are used without proper headings in some cases The preloader section on line 42 doesn't have a heading The modal content should probably use dialog instead of section for better semantics

  2. Navigation Issues The menu structure should use nav instead of just <div class="menu"> since it's the main navigation The footer menu should also use nav instead of just a ul.

  3. Article Content The news cards should use article tags instead of a tags since they represent self-contained content, while the a tags could then be nested inside the articles for proper linking.

<article class="card">
  <a href="#" class="card__link">
    <div class="card__body">
      <!-- card content -->
    </div>
  </a>
</article>
  1. Form Labels The forms lack proper label elements for form controls Using span instead of label is not semantic

  2. Button vs Link Some elements using the "button" class which function actually as a links and vice versa.

2. Are there mistakes in the names, in the usage of the blocks, elements, and modifiers?

Inconsistent Element Delimiters - Some blocks use single underscore while others use double underscores for elements. title here is not a modifier, it's an element of the card block.

.card_title {
  /* styles */
}

@media screen and (max-width: 1023px) {
  .card_title {
    /* styles */
  }
}

@media screen and (max-width: 767px) {
  .card_title {
    /* styles */
  }
}

According to the BEM methodology, card__title should be used consistently for all elements within the card block.

Avoiding mixing of BEM and non-BEM classes, adhering to the BEM convention consistently throughout the codebase.

card__body (✅) card_date (❌) - should be card__date card_text (❌) - should be card__text card_title (❌) - should be card__title card_src (❌) - should be card__src card_wrapper (❌) - should be card__wrapper

CSS Nesting according to the BEM methodology allows using nested selectors, but we recommend minimizing their use. Nested selectors increase code coupling and make reuse impossible.

Inside the auth-form__error-message_usr-exists block,

.auth-form__error-message.auth-form__error-message_usr-exists {
  text-align: center;
  display: inline-block;
  position: relative;
  top: 10px;
}

Block names should always use kebab-case, if more than one word is needed. auth-form, save-title. etc.

CSS inside of the wrapper.css file should use the BEM methodology

Avoid too many modifiers

3. Are there mistakes in the BEM file structure? Explain in detail.

The BEM file structure is not being followed consistently. The BEM methodology is a naming convention that helps developers better understand the relationship between the HTML and CSS in a project. It stands for Block, Element, Modifier.

For Example:

menu/ (block)
  __item/ (element)
    _state/ (modifier)
      menu__item_state_current.css
    menu__item.css
  menu.css
  menu.js

Based on the CSS given in this task, the BEM file structure should be as follows:

card/ (block)
  _title/ (element)
    card_title.css
  __body/ (element)
    card__body.css
  _date/ (element)
    card__date.css
  _text/ (element)
    card_text.css
  _src/ (element)
    card_src.css
  _wrapper/ (element)
    card_wrapper.css
  card.css
  card.js

should be structured as follows:

card/ (block)
  __title/ (element) Corrected
    card__title.css
  __body/ (element)
    card__body.css
  __date/ (element) Corrected
    card__date.css
  __text/ (element) Corrected
    card__text.css
  __src/ (element) Corrected
    card__src.css
  __wrapper/ (element)
    card__wrapper.css
  card.css
  card.js

The case for the _white in the blocks/header could be considered a modifier, but it's not consistent with the BEM naming convention. It should be header_white.css or header--white.css if it's a modifier.

Combining the Flat and Nested Structure is called Flex in the folder structuring convention. The CSS in the header_white.css modifies the header block to have a white background and gives a min height.

4. Analyze the adaptive design and comment on the decisions that were made in this code.

I do not have so much knowledge about the adaptive design, but I can give some general feedback.

  1. The design is responsive and adapts to different screen sizes using media queries. I noticed the Breakpoint Strategy The codebase consistently uses three main breakpoints: Desktop: > 1023px (default) Tablet: ≤ 1023px Mobile: ≤ 767px Extra small: ≤ 374px (used occasionally) This is a great decision that covers major device categories.

Responsive Grid System for the new layout. In desktop view, it uses the 3 columns, in tablet view, it uses 3 columns with reduced gap and margins, and in mobile view, it uses 1 column. Maintaining good readability and usability across different screen sizes.

Fluid Typography is noticable in the design i.e the typograpgy scales down proportionally considering the .header__title with font-size from 3.75rem to 2.25rem.

Paddings and Margins are adjusted according to the screen size. For example, the .card .client block has different padding values for different screen sizes.

.client {
    padding: 80px 104px 72px 104px;  /* Desktop */
    padding: 30px 40px 40px 40px;  /* Tablet */
    padding: 20px 16px 35px 16px;  /* Mobile */
}

Mobile first design decisions are obvious in the codebase.

Layout shift are handled well in the design. Consider the .footer

Desktop: Single row flex layout Mobile: Wrapped layout with adjusted heights and spacing

Image sizing is adjusted according to the screen size. For example, the .card_img block has different height values for different screen sizes.

Desktop: 272px height Tablet: 150px height Mobile: 196px height

Mobile menu is utilized by converting from horizontal to veritical navigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment