Skip to content

Conversation

johnkaza
Copy link

npm run dev

Runs the app in the development mode.

npm run build

Builds the app for production to the dist folder.

npm run test

Launches the test runner in the interactive watch mode.

Copy link
Collaborator

@padelis padelis left a comment

Choose a reason for hiding this comment

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

Hey @johnkaza! Thanks for your time and effort in doing this! If you may, go through my comments.

);
};

export default LinkRoute;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm why you didn't used Link directly?

Copy link
Author

Choose a reason for hiding this comment

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

hey! I always wrap to components the libraries(mui) and native components(vuejs, vue-router), because in the future we might need a big change, that way they can be changed from one place instead of replacing those components in each page one by one.

Comment on lines +4 to +10
interface ButtonProps {
variant?: 'contained' | 'outlined' | 'text';
text?: string;
sx?: object;
onClick?: any;
loading?: boolean;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the expectations when everything is optional? What do you think of it? It's easier to consume?

Copy link
Author

Choose a reason for hiding this comment

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

By mistake I put the text and the onClick as optional. Basically the concept is to keep mandatory only the one's that will be absolutely mandatory like text and onClick. properties such as sx, variant and loading can be optional because Mui Button has default properties on those properties. Text and onClick don't have a default state and they are needed for the button to function properly.

Comment on lines +4 to +5
const TOKEN = 'live_u0HsGpuot5dyUPhzWvS2OiWzh7lVUADeDrRSWNCCrWcfBemDGqHcKIFDdRknvlGZ';

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you were to deploy, how would you handle this?

Copy link
Author

Choose a reason for hiding this comment

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

I would have created .envs.[mode] for each environment in order to build the project.

if we had 3 environments: development, staging, production the files would be likes the example below:

.env.development .env.staging .env.production

and on the package.json it would be

vite build --mode <enviroment_name>

plugins: [react()],
test: {
globals: true,
environment: 'jsdom',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tried any other envs?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't tried other environments than jsdom for browser emulation but i know that there is also happy-dom environment where it is faster and more lightweight than jsdom when it comes to emulate browser environment for testing components. happy-dom is being used for simpler tests where jsdom is more compatible for advanced test cases. if we wanted to test only utilities where there is no DOM iteraction, we would have used node enviroment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

empty? 😅

Copy link
Author

Choose a reason for hiding this comment

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

ahaha well I'm sorry, I know it looks bad but I was going to create a helper for the localStorage to be used in the useFavorite.tsx but I was running out of time. Wrapping the localstorage in a wrapper helpers function would help if anything is needed to change in the future to be changed from one place instead of many places. Helpers can be even broken down to each individually helper file so it can be more scalable and maintanable.

example:

// localStorage.ts

// Wrapper for localStorage.setItem
function setLocalStorage(key, value) {
  try {
    // Convert value to a JSON string before storing
    localStorage.setItem(key, JSON.stringify(value));
  } catch (error) {
    // if localStorage is not supported, we would have a fallback functionality or send a message to the client that the 
   // browser is not supported. or even send analytics which browser the user had when the error happend

    console.error("Error setting localStorage item", error);
  }
}

// Wrapper for localStorage.getItem
function getLocalStorage(key, defaultValue = null) {
  try {
    const item = localStorage.getItem(key);
    
    // Return parsed value, or defaultValue if key is not found or is null
    return item ? JSON.parse(item) : defaultValue;
  } catch (error) {
    console.error("Error getting localStorage item", error);
    return defaultValue;
  }
}

// Wrapper for localStorage.removeItem
function removeLocalStorage(key) {
  try {
    localStorage.removeItem(key);
  } catch (error) {
    console.error("Error removing localStorage item", error);
  }
}

// this can be a also test function for the localstorage to ensure that the localstorage works on the browser the user // is using.
function isLocalStorageAvailable() {
  try {
    const testKey = '__test__';  // Key used for testing
    localStorage.setItem(testKey, testKey);  // Try setting an item
    localStorage.removeItem(testKey);        // Try removing it
    return true;  // If no errors occur, localStorage is available
  } catch (error) {
    return false;  // If an error occurs (e.g., due to privacy settings or limitations), return false
  }
}

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.

2 participants