Looking to hire developers?
@antarya
@VishwajeetBarve
Submitted
This is the first time I have used ChartJS in any project, so I had to learn the basics of it before I could get started. Overall it was a fun experience plus I will be looking to do projects of this type i.e Data Representation.
Please go through my solution and if there are any suggestions/tips then let me know. Thank you.
@antarya
Posted
Hi 👋,
Good work on ChartJS usage 🚀.
I have a couple of suggestions/improvements:
HTML
[1. Any reason you load bootstrap.grid.css and a bunch of fonts that are not used anywhere?
[2. I think headings are misused. For inspiration, look at other solutions, e.g. https://www.frontendmentor.io/solutions/responsive-bar-chart-component-with-json-data-fetching-b2LJ5eTGIX.
[3. There is a typo on line: <div class="container" width="100px" ,height="100px"> you need to remove comma. Also, I do not think setting width and height is valid; you can specify it as style but not attribute.
JS
[4. You have a typo on this line events: ["mousmove", "click"],, it should be mousemove; otherwise hover effect does not work. Do you have any specific reason to limit events?
[5. To match the design, you can remove the horizontal line below the bar element:
x: {
grid: {
display: false,
// Add this line to remove the bottom line
drawBorder: false,
},
}
[6. The y-axis configuration is set twice, which means only a second try will be preserved.
{
scales: {
...
y: { // this will be lost
grid: {
display:false
}
},
y: { // this will stay
display :false
}
}
}
You can specify all options like this:
y: {
display :false,
grid: {
display:false,
}
},
[7. To make sure border-radius is set everywhere, you can use the borderSkipped option:
options: {
borderSkipped: false,
...
}
Check API how it works https://www.chartjs.org/docs/latest/charts/bar.html#borderskipped
General
[8. To name your classes consistently, check BEM, and to structure it in a reusable way, take a look at CUBE CSS. Check these resources for additional information:
[9. Use CSS reset to have a good starting point, e.g. https://piccalil.li/blog/a-modern-css-reset/.
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful
@damiandev22
Submitted
I think the eye icon on the overlay image is not well centered, How can I solve this? Is ok the opacity of the eye icon?
any other suggestion will be well received.
thanks
@antarya
Posted
Hi 👋,
Good work on this challenge 🚀.
I have a couple of suggestions/improvements:
HTML
[1. The anchor element cannot have another anchor element as a child.
CSS
[2. As .overlay has already correct position and size, one way to center child elements would be:
.overlay {
display: flex;
justify-content: center;
align-items: center;
}
The icon itself does not need any styling in this case except the colour.
[3. While the solution for centering above is different, it would be helpful to understand the cause of the previous issue.
The child anchor element's height is calculated using font, font-size and line-height, and then img is placed on the text baseline. You can try to change vertical-align on the image to see how it changes the position. The height is huge compared to the image, and the image is placed similar to how the text would be placed. You can, for example, fix that by setting font-size: 0 on the anchor element, but in your case, you just remove the child anchor element. Here is related material:
[4. When the viewport height is small, the card is only partially visible, and there is no scrollbar. You can change the body element height style to min-height. Also, a bit of padding applied to the main element will add space otherwise on smaller screens it is on the edge of the viewport.
General
[5. Use css reset to have a good starting point, e.g. https://piccalil.li/blog/a-modern-css-reset/.
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful
@AryanTy20
Submitted
first time trying with typescript any suggestions ?
@antarya
Posted
Hi 👋,
Nice job 🚀. I noticed a couple of things that can be improved.
HTML
[1. Use button tag instead of div tag for icon buttons, and do not forget to add aria-label for buttons with no text.
[2. Usually, the logo is a link to the home page with an alt property set to the app name.
[3. Check responsiveness on screen size e.g. 692px.
JS/React/Typescript
[4. Typescript looks good; when possible, create reusable types, e.g.:
interface Image {
poster: string;
thumbnail: string;
}
And use it whenever this type is used.
[5. If you use hard-coded data, move it out of the component, nothing is happening with it while rendering. A better approach would be to imitate fetch using a slight delay with a loading indicator to mimic the real application.
[6. The first parameter of the array splice method is the index at which to start changing the array. Using id to remove the item from the cart as the first parameter of splice will not work correctly.
[7. You can skip <>...<> as the root element when there is only one child element.
[8. Use the header tag as a root element of the Header component instead of wrapping the Header component in App.
[9. In Cart component you check if cart is empty several times, it is a good candidate to have it in a variable e.g. const isPresent = data?.length > 0.
[10. useState<boolean>(false) you can skip type here if you expect the value to be only boolean as typescript will know.
[11. NavLinks will probably be reused in other places .e.g in the footer, so keep it outside the component and/or move it to its file.
General
[12. Use reset to have a good starting point, e.g. https://piccalil.li/blog/a-modern-css-reset/.
[13. For commit messages, it will help others if you add a descriptive message and follow a specific format, so all is consistent https://www.conventionalcommits.org/en/v1.0.0/. You can even make a check before committing if you follow those rules by using a tool like commitlint - https://commitlint.js.org/#/
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful
@Andrusik1
Submitted
Can i optimalize that ?
@antarya
Posted
Hi 👋,
Nice job 🚀. I noticed a couple of things that can be improved.
HTML
[1. Do not use main tag as a wrapper of card, instead add parent div or section tag for your card <main><div class='panel'>...</div></main> as a child of main tag. main tag specifies the main content of a document https://www.w3schools.com/html/html5_semantic_elements.asp
[2. Rate-related elements with submit button can be recreated and wrapped with the form, which is a better way to handle this case. Instead of a button for each rate, you can use radio input as it matches the required logic choosing one from several. You will learn a lot by checking other solutions after your attempt, e.g. check the solution for the same challenge of @Elaine https://www.frontendmentor.io/solutions/responsive-interactive-rating-component-SeBo-aR4gB it is implemented using form and radio inputs; it has consistent names and implemented well.
CSS
[3. The main tag used as a card wrapper has a width: 25vw without restriction on huge screens; your card will be pretty wide. Instead, you can set the width to 100%, but restrict it not to be greater than 400px using max-width: 400px.
[4. It looks like you are using BEM for naming classes; if yes, I think it is not used correctly. For example .Card__starbox--img - img is not a modifier but an element, instead you can use something like card__sticker, same for .Card__Submitbox--submit. .Card__Rating---rate has three dashes, and .second-image is not part of the card - consistency will make your code easy to read and maintain.
JS
[5. You don't need to apply styles from javascript in this case. Define them in CSS and use the class name to reference those styles.
[6. For a map of a similar key/value in the getRate function, why not use Amount directly?
[7. While there is nothing wrong with string concatenation, you can also check template literals.
[8. Getting the answer value can be moved into submit click listener, as it is needed only on submit, not before.
General
[9. Use reset to have a good starting point, e.g. https://piccalil.li/blog/a-modern-css-reset/.
[10. There is only one commit which might be fine for a small project, but even on a small project, train yourself and commit often with a meaningful message when specific work is completed. For a consistent format of commit messages, you can use (https://www.conventionalcommits.org/en/v1.0.0/).
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
@tossik8
Submitted
I used CSS grid for this task and came across a problem. When I made the grid layout for 375-px-wide screens I changed the number of columns to 1. For desktop devices I had 2 columns, and expected that setting grip-template-columns to 1 would change it. However, it did not work. All my sections are in 1 column as I intended them to be, but there is another column, which does not contain anything, and it creates extra redundant space on the right which I don't know how to remove. The question is, how do I remove that column?
@antarya
Posted
Hi 👋,
Good work on this challenge 🚀.
CSS
[1. Regarding your question. You have width: 50% on body main .elements which sets elements width to 50%. The grid works as expected; it is not an extra column but one with elements width set to 50%. In Chrome, there is a grid badge/toggle under the Elements tab of DevTools for the elements with display: grid; by toggling it, you will see the outline and additional info about the grid.
[2. Use CSS reset to have a good starting point, for example:
[3. You will have issues if you customize styles for specific text lengths. You need to make a layout adaptable to any text length, as the length of the text is unpredictable. Start by removing fixed widths, allow elements to grow and restrict using (max|min)-(width|height) when needed; remove unnecessary margins.
HTML
[4. Do not use main tag as a wrapper of your panel, instead add div tag for your panel <main><div class='panel'>...</div></main> as a child of main tag.
General
[5. Take a look at a different approach where you first define default styles for narrow screens and add features for wider screens.
[6. To name your classes consistently, check BEM, and to structure it in a reusable way, take a look at CUBE CSS. It will allow to reuse styles or make them easily customizable without deep nesting. Imagine a situation where you need to inject another element before body main .elements:nth-of-type(2) button { this will require changes that are not straightforward, compared to if you had .panel__subscribe button, or even better to style .button so it can be reused. Check these resources for additional information:
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful
@mujpao
Submitted
Is there any way to implement this design without using the placeholder attribute?
@antarya
Posted
Hi 👋,
Excellent work on this challenge 🚀. It looks fantastic on different screen sizes.
[1. Regarding your question. You can use a label on top of the input, and when the input is focused, either hide or move the label to a different place. You can check the real example here https://mui.com/material-ui/react-text-field/#basic-textfield; inspect input to see all elements.
[2. While using the screen reader, the input error is read only once. Later, if you focus on input with an error, there is no indication that the input is invalid. Using aria-invalid and aria-errormessage or aria-describedby might help with it https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-errormessage.
[3. Submit event listener is added for each input; is it intentional?
[4. When input text is long, and the error icon is visible, the text overlaps with the icon. Increasing padding on the right side will help to fix it.
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful
@alexanderbonney
Submitted
Any feedback is welcome.
@antarya
Posted
Hi 👋,
Nice job 🚀. Noticed a couple of things that can be improved.
CSS
[1. The header is not showing all links in some cases:
[2. When the mobile modal is open, navigation links do not expand to show inner child elements.
[3. Element with .top class has min-height: 80vh, on bigger screens text in the hero section is not centred vertically. Also at some point, if you start decreasing height, the login button of the hero section will be partially invisible. I think removing min-height and adding bottom padding will do the trick.
[4.Start for Free like buttons have no pointer cursor and lack the animation you have on the Login button.
[5. main section.content .contentBox .content_right .imgBox.image-left is a bit long and redundant, you might benefit from using BEM https://getbem.com/naming/ e.g. info-section__image and .info-section__image--left.
[6. Spaces/sizes are a bit off; if you use a slider on the solution page, you can see the difference.
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful
@fawzab
Submitted
All feedback is welcome
@antarya
Posted
Hi ,
Great job, it looks good.
I have a couple of suggestions:
CSS
[1. All elements inside .whole have margin-left: 40px. To achieve the same but to have only one style, you can define padding on the .whole element. It will be easier in the future to change it instead of specifying it for each child element.
/* remove all margin-left: 40px on all child elements and add one padding to parent */
.whole {
padding: 17px 40px;
}
[2. The content inside the Add to Cart button is not aligned vertically; it looks like the fixed height is redundant.
.whole .fa {
/* width: 50px; */
/* height: 100px; */
margin-right: 20px;
}
[3. You have fixed width and height which works in this case, but you cannot depend on text length; in real applications, you never know the size of the length, so you need to make your layout adapt to whatever length it will have. You can check it by typing longer text for an item description. Allow elements to grow without fixed width/height, and when you want to restrict, use max-width/max-height or min-width/min-height;
[4. On smaller screens, e.g. 320px, the page doesn't look like in mock. You might need to use media queries, so it looks good on smaller screens.
And for arranging items, try using flexbox or grid as it is easier to use and has more features.
Using a mobile-first approach and media queries, you will change styles to:
.container {
/*
A
B
*/
display: flex;
flex-direction: column;
/* you can set radius on parent element instead of each child element */
border-radius: 10px;
overflow: hidden;
}
@media (min-width: 900px) {
.container {
/*
A B
*/
/* display: flex; */
flex-direction: row;
}
}
[5. You can set display: block on the image to remove white space under the image. For details, check:
HTML
[6. Wrap your card container with the main tag instead of using the main tag as your card parent tag; this would be semantically correct.
Let me know if you have questions. I hope this will be helpful.
Keep up the good work . Cheers!
Marked as helpful
@debadutta98
Submitted
need your feedback for further improvements. solution
@antarya
Posted
Hi 👋,
Great job, it looks really good.
I have a couple of suggestions/improvements, though:
HTML/CSS
[1. Country flag images look distorted on the main and the details page. On the main page, you can remove height so it can grow based on width. And the details page, instead of specifying height, you can remove the height attribute and limit max-width to a value of 400px, for example.
[2. The class names are a bit long; it will be hard to work with them in the future. As you are already using BEM (http://getbem.com/naming/), instead of
country-details__content__details__information
you can have
country-details__infocountry-details__content__details__information__container-1
country-details__info-maincountry-details__content__details__information__container-2 - country-details__info-secondary[3. As far as I understood, the border country elements on details pages should be a link to a country itself.
JS/REACT
[4. For HTML Validations error that can be seen in the report, I think all is related to illegal characters in href, which you can fix by wrapping URL params in encodeURI.
(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI,
https://stackoverflow.com/questions/332872/encode-url-in-javascript)
[5. For numbers to be user-friendly, format them so that instead of 35000322, you will have 35,000,322. (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat)
[6. When you search for a country, and the result is empty, it shows the last result. Should not it be an empty list?
[7. It is better to be consistent and use one format for variable names. For example, you have filterref, searchRef, and country_results. (https://javascript.info/variables#variable-naming)
[8. Using constant for API URL or helper method will allow to change it quickly, and it will not be hardcoded in multiple places.
const API_URL = "https://restcountries.com/v2/";
fetch(`${API_URL}/name/${param.countryname}?fullText=true`);
[9. The value of mode is passed to each component that needs it. There is a term for it prop drilling, which is okay in some cases, but as this value is shared across all application components, you can use Provider Pattern (https://www.patterns.dev/posts/provider-pattern/) instead, which is using Context (https://reactjs.org/docs/context.html). Using this pattern will remove unnecessary duplication and be more maintainable. styled-components has a helper component to do precisely that - https://styled-components.com/docs/advanced#theming.
[10. Regarding variable name mode - it is hard to understand the meaning without reading the code; instead, you can name it isDark | isLight | theme if it has 'dark', 'light' values.
[11. In MainContent, you are using ref for input. Why not directly use useState (https://www.w3schools.com/react/react_forms.asp)?
[12. For the select field, you will benefit from using libraries like https://react-select.com/home as it is flexible and follows best practices.
[13. You can use useMemo to avoid unnecessary processing/calculations and be more performant. It will only recalculate the value of filteredCountries on filter or country_results change. Example:
// https://reactjs.org/docs/hooks-reference.html#usememo
const filteredCountries = useMemo(() => {
if (filter) {
const filterValue = filter.toLowerCase();
/* You can also prepare lowercase region name on the cleaning stage, so it is done only once and not on each call of useMemo */
return country_results.filter((value) => value.region.toLowerCase() === filterValue);
}
return country_results;
}, [filter, country_results]);
...
<Grid className="grid">
{filteredCountries.length > 0 && filteredCountries.map((country, countryIndex) =>
<GridCard result={country} key={countryIndex} mode={props.mode} />
)}
</Grid>
[14. I noticed that data is fetched every time user navigates to country-api. What do you think of getting all data once and then reusing it?
GENERAL
[15. For some reason, when you reload the detail page, it shows a 404 error.
[16. You can use prettier, which can format your code automatically on save, so code formatting is consistent. For example, you will not have two different formats:
const CountryInDetails = ({ mode }) => {
const NavBar=(props)=>{
https://create-react-app.dev/docs/setting-up-your-editor/#formatting-code-automatically
[17. For commit messages, it will help others if you add a descriptive message and follow a specific format, so all is consistent (https://www.conventionalcommits.org/en/v1.0.0/). You can even make a check before committing if you follow those rules by using a tool like commitlint - https://commitlint.js.org/#/
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful
@leoenter
Submitted
What's your opinion ?
@antarya
Posted
Hi 👋,
Great job, it looks really good.
I have a couple of suggestions/improvements, though:
HTML
[1. Wrap your content with the main tag. That way browser will know where to find primary content. Also, it will fix some accessibility issues. Here is a resource with examples related to semantic tags.
[2. Add alt attribute to img tag. It can be empty or should describe what is on the image. Screen reader users should have an idea of what is on the image based on the alt text. Here are some resources: mdn alt, great-alt-text.
[3. HTML and CSS files are not formatted properly. It will make your code more readable if you use an editor extension that can format on each save for you. For example, if you are using vscode try prettier, for different editor search for editor-name prettier.
CSS
[4. Consider more universal approach for box-sizing using this method.
[5. Take a look at this thread regarding different ways to name CSS colour variables, so it is easier to use while styling your app. slack discussion.
[6. You are using :nth-of-type(N) pseudo-class a lot, which looks not very efficient. Next time you want to insert another card in the middle, you will have to change the rest; instead, you can create .card--bg-pink or .bg-pink and apply where necessary. Another example would be to have modifier .card--with-quote-bg, which will add quote background. That way, you make your code easier to read; and easier to apply changes. Here is a resource on how to structure your CSS using [BEM methodology] (http://getbem.com/introduction/), also later, you might be interested in this scss and this tailwindcss.
[7. Where possible, try not to use fixed width and height so your elements can adapt to different screen sizes, limit using max-, min- version of width and height where required but allow to adapt. Also, it feels like not the best choice to use em, unless that was the intention to bond width to font size. One of the possible solutions using grid but without fixed width/height would be to remove all width/height except on containers and then have something like this:
/* For this to work first in HTML, move current card#5 to be card#3 so it is ordered the way they appear. */
.card {
height: 100%;
}
@media(min-width: 1200px) {
.container {
padding: 20px;
}
.grid-wrapper {
display: grid;
grid-template-columns: repeat(4, 1fr);
grid-gap: 20px;
}
.card:nth-of-type(1) {
grid-column: 1 / 3;
grid-row: 1;
}
.card:nth-of-type(2) {
grid-column: 3;
grid-row: 1;
}
.card:nth-of-type(3) {
grid-column: 4;
grid-row: 1 / 3;
}
.card:nth-of-type(4) {
grid-column: 1;
grid-row: 2;
}
.card:nth-of-type(5) {
grid-column: 2 / 4;
grid-row: 2;
}
}
There are other ways to achieve this, e.g. using grid-template-areas.
[8. It would look better if you move background-color to the body tag, so all area is filled with background.
[9. Avatar size is not equal as it has width: 12%, so it depends on the size of the container. In this case, you might benefit from using fixed sizes as you know it should have the same size on all cards.
.img-div {
width: 45px;
height: 45px;
}
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful
@carlwicker
Submitted
Fun project. Feedback welcome.
@antarya
Posted
Hello Carl,
Great job, it looks really good.
Couple ideas/suggestions/improvements for your implementation:
JS
[1. The if/else check of window.innerWidth looks redundant you may simplify by:
// original code
if (window.innerWidth > 1439) {
setIsMaxWidth(true);
} else if (window.innerWidth <= 1439) {
setIsMaxWidth(false);
}
// instead you can write this
setIsMaxWidth(window.innerWidth > 1439)
[2. From the code, it looks like menuHidden, isMaxWidth, tabs is only used inside specific components, so why have them outside then? My rule is to put things as close as possible where they are needed. Also, moving the tabs variable outside the component will not recreate it on each re-render; it is static data that does not depend on anything.
The same goes for desktopHeros and mobile mobileHeros. Put it outside of the components. Also, there is no need for those variables as the only thing that is changing is the number that you can get from the slide number.
HTML
[3. Where possible, use existing elements, e.g. instead of div to mimic buttons, use the button tag and restyle it as you need. That way, it will retain native browser behaviour. For lists, use ul/li; other elements that can be used are header, nav; take a look at semantic elements and why they are used html-semantics-cheat-sheet.
CSS
[4. Regarding responsiveness, it looks strange to see one small column on the desktop when the screen is 1400px in width. You are heavily using fixed width and heights maybe using img tags and max-, min- versions of width/height may help to make it adaptable to other screen sizes.
[5. While there is nothing wrong with using BEM and CSS modules together, it looks redundant to me. With CSS modules, there is no need to scope class names as they will be already unique. That is, the whole point css.button in one component is different than css.button in another component. Also, you may use some nesting like .about h2 {}. You will get rid of the css[''] style as you can easily find simple/short names for all scoped elements. Take a look at other examples css-modules-examples.
If you find yourself using a lot of conditions inside className, you may take a look at this library clsx, or classnames.
React
[6. As the project's design suggests, there should be other pages; having this in mind even for small projects will help later. For example, having a pages folder with the LandingPage component and moving all related components inside. Or building a general Layout component and using it inside LandingPage as the wrapper.
[7. In App component window.addEventListener("resize", will be called on every render, which means you are adding more event listeners on each re-render which I guess was not your intention. If you move that code into useEffect(..., []), this will be called only once on the mount, also in useEffect, you can remove the event listener on component unmount using the return method. For inspiration, you may check this library of hooks and particularly react-use which does the same thing as you want.
[8. In the TopBar component, instead of className conditions everywhere, you may set className on the parent like ...--desktop, and for all that need the same check, use that like ...--desktop .topbar__nav-items inside CSS.
[9. As HeroImage and PageContent share state and data (as each slider image has different text), you might combine them inside the wrapper component.
===
I hope this will be helpful.
Keep up the good work!
Marked as helpful
@Da-vi-de
Submitted
Hello community, that's my first FEM project in React and the second i ever made using this library. It has been a long journey, i'm satisfied with the result but i focused most of my attention and energy on the functionality part (it kept me very busy) of the project. I decided to use plain CSS, i tried to avoid repetition except for the theme switcher, i couldn't avoid WET code which means that maybe the whole functionality could have been implemented in a better way.
I thought it was nice having a load more button and a back-to-top button so i added them. I could disable the load more button at the end of the page but just for all countries, it would be nice making it work for each filter, i'm not there yet.
I've never used select element, it turned out to be very hard to style especially in Google Chrome it appears worse than Firefox, i tried my best! Dark mode looks ok on mobile but on desktop the arrow down disappears.
The API changed. That happened nearly at the end of the project, the new endpoint was unstable for over a week, at the moment is working great! However there's an important difference between the two. The old API had the border property in all countries, no matter if a country has no border country, the property was there and of course the value was 0, i structured my code so that it worked perfectly! The new API didn't include the property in all of those countries with no borders, and that meant i had to change the code structure to make it perfect, so you are going to see a blank page when you click some countries.
To conclude, altough there's room for improvement i'm quite happy i could work on an advanced challenge, i learned a lot and i want to learn more about React, patterns, architecture and styled components is the next goal.
Feel free to leave a feedback.
@antarya
Posted
Hello Davide,
Great start with React 🚀.
I like the idea of the load-more button that you added 👍. Couple ideas/suggestions/improvements for your implementation:
Issues
[1. If you search for a country that is not yet visible, the list will be empty. It is happening because there is a slice of data before filtering. Changing the order of slice and filter should do the trick.
[2. When country borders is not defined single-country page raises an error. For starters, you can assign an empty array, so it is working.
const {
alpha2Code,
...
// if borders is undefined it will set it to empty array
borders = []
} = country;
Take a look at Destructuring search for "Default values" for an object.
[3. The load more button stays on the page, even when filtered data has less data than the visibleCards amount. A simple condition will fix that.
[4. If you reload the single-country page or navigate to it directly, there is an error in the console. That is caused because at this point country list is not loaded yet.
React
[5. Countries page is calculating countries variable every time it renders. The trigger of render might be something other than search/filter/visibleCard change. One of the ways to eliminate unnecessary calculation is to use useEffect({...setFilteredCountries(...filtered countries) }, [query, fitler, visibleCards]). And inside component return {filteredCountries.map((c) => <Card ... />)}.
[6. As far as I can see, the API does not support pagination, but it supports fetching specific country/countries. For the single-country page, there is no need to know about all countries. You might load specific country data directly and after it border countries; even better before that, checking context if all countries are already loaded and if yes, get it from there instead. What I am saying is that fetching all countries on the single-country page is redundant.
[7. Currently, a global context has data related to the theme, country list, query, and filter. If we check the usage of those, it would be: theme: whole application country list: countries page/maybe single country page query: only countries page filterData: only countries page
It looks like only theme/maybe countries should be accessible by the whole app, and the rest can be moved to a specific page. If you keep adding new pages to your app, those pages will probably not need to fetch allCountries, and they do not need to have query/filterData. The simplest solution would be to move query/filterData state to Countries page, and trigger fetching all countries from Countries page but save it in context as you have it right now. And for the single-country page to check if allCountries is already loaded, use that if not - fetch single country data.
[8. It would be nice to have a loading indicator while filtering/loading more and fetching single country data.
[9. You might reset visibleCards to the initial value every time you use a query or filter.
[10. On the Countries page, the same logic is executed for country filtering. You can combine those:
if (item.region === filterData) {
...same
}
else if (filterData === "All") {
...same
}
// instead you can do
if (filterData === 'All' || item.region === filterData) {
...same
}
}
Also, you can improve it by checking query value. There is no need to use a query if it is an empty string.
[11. main tag element and `back-to-top' are excellent candidates to move to separate component and reuse on all pages or move it to the app component. e.g.:
const Layout = ({children}) => (
<main className="main-content">{children}</main>
<BackToTop />
)
[12. const [search] = useState(["name"]); - there is no need to have it as a state you can have it as a const out of the component. e.g.: const searchFields = ['name'];
[13. There are both versions in code; I suggest sticking to the last one as you are not repeating yourself.
<i className= {darkMode ? "fa fa-search icon darkmode" : "fa fa-search icon"}></i>
<i className={`fa fa-search icon${darkMode ? "darkmode" : ""}}></i>
If you find yourself using a lot of conditions inside className, you may take a look at this libraries clsx, classnames
JS
[14. Instead of classList.add() and classList.remove() you can use classList.toggle("className", flag)
[15. Things like the number of items per page are a good candidate to move to a constant and out of component to be reused, e.g. const COUNTRIES_PER_PAGE=49.
[16. There is no need to call query.toLowerCase() each time you check if it is part of the country name. As query is not changing, set a variable before filtering and use it instead. It might be fast now, but there can be some heavy calculation involved in the called method in other applications.
HTML
[17. Currently, users need to guess that card is clickable unless the user hovers the image. If you make the whole card clickable, it might be better for the user experience.
[18. The usage of semantic elements like footer and article is misleading. I would make a footer child of body and place content inside, and the article will contain all country info.
[19. Instead of alt="Country flag route", you can change it to include country name so that the screen reader will read to which country user will be navigated.
[20. Change theme switch label when changing theme for better user experience Light Mode/Dark Mode.
[21. To write less instead of writing lists by hand:
<option value="All">Filter by Region</option>
<option value="Africa">Africa</option>
<option value="Americas">America</option>
...
// you can create an array of regions
const regions = ['All', 'Africa', 'Americas' ...];
// and later output
{regions.map((region) => (<option key={region} value={region}>{region}</option>)}
// cleaner code, easier to update, you can later use API to get all regions
CSS
[22. The issue with the select input arrow icon can be fixed by removing background-image: inherit; from .select-dark, and linear-gradient from:
select,
.select-dark {
/* replace this */
background-image: url('data:image/svg+xml;...'), linear-gradient(to bottom, #ffffff 0%,#fff 100%);
/* with this */
background-image: url('data:image/svg+xml;...');
}
That should do the trick. In general, it is hard to style select input. Design of select input is much more difficult most of the time, so you will be forced to use an external library. Here are examples of libraries that you can use instead: For react react-select For js select2
[23. You are already using the theme class on the body tag instead of having theme-related classes on each element. You can reuse parts of element styles that are shared and apply only differences based on body theme class.
.btn {
/*
all shared styles
and one for the light theme
*/
}
.dark .btn {
/*
specific to dark theme
*/
}
By doing that you can remove code like <div className= {darkMode ? "country-info-dark" : "country-info"}> as you will just have <div className="country-info"> and body theme class will trigger theme specific overwrites.
[24. There are minor differences when switching from light to dark:
.country-detail > p font-weight is changing to lighter for some reason.Resources
To format code and be consistent, you can use Prettier. Also, you can add configuration so every time you commit, it formats it CRA-Formatting Code Automatically
Take a look at another style for className naming format BEM
As I said only have a couple of suggestions 😬😅. I hope it is not overwhelming and will be helpful.
Keep up the good work! Cheers!
Marked as helpful
@shivjoshi1996
Submitted
Hi everyone,
I used React (currently learning) to build this app. I found it quite challenging due to being new to React and not knowing how to do certain things that I know how to do in vanilla JavaScript.
A few questions that would really help me out:
What is the best way to show which button has been selected? I thought about storing something in the state of the "tip selection" component, but I'm not sure what I should be storing, and how I would dynamically change the colour based on which button has been stored in state (would I change the prop of the selected button somehow?)
Any improvements in how I have organized my React code would be great, primarily the way I am utilizing components and props (I know my styled-component and JS code looks quite messy in the calculator component).
I also got ESLint errors "JSX Props should not use functions", but I'm not sure how else I would achieve this functionality without sending some of the functions down to child components. Any ideas on how I could avoid this error?
I didn't get around to adding the error states, opting to try to conduct validation in the input fields instead. If I were to add error states, would I simply use state to check if the input is valid (turning state to either true or false), and then use a conditional in the JSX to display the error message if state says the input is invalid?
Any other comments would also be appreciated!
@antarya
Posted
Hello Shivam,
The app and code look neat. Excellent job 👍.
[1. One way of doing it is, as you suggested, to have a state for selectedTip in the TipSelection component, but that should not be a number as you will have no ability to distinguish between a button tip and a custom tip. Later based on the selected tip, you can check for each button/input if it is active and style it accordingly. It would also be good to switch to custom value every time custom input is clicked/chosen.
Here is a snippet with the initial code:
const tips = [5, 10, 15, 25, 50];
export default function TipSelection({ handleTipInput }) {
// make sure you preselect tipAmount in Calculator
const [selectedTip, setSelectedTip] = useState(`tip-${tips[0]}`);
const handleCustomFieldChange = ({ target }) => {
const { value } = target;
setSelectedTip('custom');
handleTipInput(value);
};
const handleTipChange = (value) => {
setSelectedTip(`tip-${value}`);
handleTipInput(value);
};
return (
<StyledTipGrid>
{tips.map((tip) => (
<TipButton
active={selectedTip === `tip-${tip}`}
handleTipInput={handleTipChange}
amount={tip}
/>
))}
<input
onChange={handleCustomFieldChange}
onClick={handleCustomFieldChange}
type="number"
placeholder="Custom"
className={selectedTip === 'custom' ? 'active' : null}
/>
</StyledTipGrid>
);
}
[2. The only idea that comes to my mind regarding the code organization is to move the input section and output sections into separate components; other than that, it looks clean. Regarding styles, it is a necessary mess.
[3. I guess it complains because on each render handleTipInput function will be brand new, so you need to wrap it with useCallback and add dependencies; that way, on each render same function(reference) will be used.
const handleTipInput = useCallback(
function (tip) {
const calculatedTip = tip / 100;
setTipAmount(calculatedTip);
},
[setTipAmount]
);
If you updated your code with the above change, wrapping your TipButton in the memo will render the component only when props change.
jsx-no-bind
memo
Regarding other Eslint errors; instead of writing
{
// eslint-disable-next-line
}<label htmlFor="tip"> Select Tip %</label>
you can write it using JSX comment instead of javascript
{/* eslint-disable-next-line */ }
<label htmlFor="people">Number Of People</label>
But better to fix using one of the ways as suggested label-has-associated-control
And for tip label, you may use aria-labelledby
[4. I guess the way you handle it depends on what is your end goal. Should it show one global message or for each input, or each input will have different errors based on different validations.
Here are some resources for more complex form validations/helpers yup and for form wrapper formik.
===
One other suggestion related to performance; There is no need for two effects that do almost the same job; why not combine into one and calculate both values there? I guess you can also combine results in one state. Otherwise, your code will be making extra render after the first run, e.g. it calls A, B(with the wrong tipAmountPerPerson) and then again B.
I hope this will be helpful.
Keep up the good work!
Marked as helpful
@Badr281
Submitted
I want to know want i miss exactly on this exercise for enhance my skills on front also some recommanded resource thanks in advance
@antarya
Posted
Hi Badr,
Great start. Nice job.
I have a couple of suggestions, though:
HTML
CSS
While your class names are pretty good, you might also want to know about another often used format called BEM.
It is a bad practice to use id for styling; the reason is that you cannot reuse it later in your code. id is used to identify elements uniquely. In more complex applications, you will have elements that will look the same so that you can apply the same class, e.g. btn.
Take a look at the mobile-first approach. The basic idea is first to take care of mobile-related styles and overwrite desktop styles using media queries. Mobile First.
Try not to use fixed width and height, instead use alternatives min-width, max-width, min-height, max-height. That way, you do not restrict your element, and it can adapt to content. In a real application, you do not control the size of the text; it will be dynamic. So it would be best if you create styles that can adapt to the content.
For example, you have height: 90vh on .checkout-card, if you start changing the viewport's height, you will notice that your content will be outside of your container at some point. Give elements to breathe and adapt, and when required, limit visually using max, min versions.
I noticed that you are using float and position to position elements of the #changeParent. I think you can do it much easier using flexbox. Here are some resources:
Flexbox Game
MDN Flexbox
CSS tricks can be used as cheatsheet
On the solutions page, you have the ability to compare results with the initial design; play with it and check things like font-family, font-color, font-size, line-height, space between elements, padding, border-radius, shadow. Eventually, you will train your eye to notice differences.
If you want the background not to repeat itself, you can use the no-repeat value on background-repeat.
For more resources, also check frontendmentor resources page.
I hope this will be helpful.
Keep up the good work! Cheers!
@mastertope
Submitted
Hi mentors,
I just came back from being dev. My current work is mostly focused on legacy code support work (VS2005, VS2013). I am polishing back my skills on par with current standards, and I decided to do that by starting this project.
Please rate my work here, I would deeply appreciate your feedback, be it negative or positive, I will take it all hehe.
Thanks!
@antarya
Posted
Hi Tope,
Great job, it looks good.
I have a couple of suggestions/improvements, though:
HTML
Wrap your content with the main tag. That way browser will know where to find primary content. Also, it will fix some accessibility issues. Here is a resource with examples related to semantic tags.
Any benefit of using the input tag to output data? What do you think of using the output tag or simply p tag?
If you click in the space between tips, the tip % is reset to 0, is that planned behaviour.
It might not be the best idea to use the name attribute to save tip value. If you want to save data on an element, you can use the data attribute, e.g.: data-tip="5". Here is a link that might be helpful Data attributes.
I think the custom tip is supposed to be input where you can enter any value.
For better user experience, highlight the currently selected tip.
For tip buttons, you might find it easier to use <input type="radio" />, as it is natively close to what you want to achieve for a group of buttons. Here is a link that might be helpful Radio Input.
CSS
Instead of using position to position some elements, would not it be easier to do it using flexbox?
For input icon, you can set pointer-events: none; so on click input will be focused. pointer-events
Take a look at the mobile-first approach. The basic idea is that you first take care of mobile-related styles and overwrite desktop styles using media queries. Mobile First.
JS
// instead of
bill_val = parseFloat(bill.value) &&
parseFloat(bill.value) !== undefined &&
parseFloat(bill.value) !== NaN ? parseFloat(bill.value) : 0.00;
// get value once and then reuse it
newValue = parseFloat(bill.value)
bill_val = newValue && newValue !== undefined && newValue !== NaN ? newValue : 0.00;
// check also is a good candidate to move as a separate function
Move formatting logic to separate function so you can reuse it.
For consistency use one naming format e.g. tipPerPerson_val or tipTotalPerPerson.
Next step to improve application can be to validate input so user can only enter numbers or instead use <input type="number"> so you have some browser-based validation
Number Input.
I hope this will be helpful.
Keep up the good work! Cheers!
Marked as helpful
@JoseVSeb
Submitted
I created the solution using plain CSS with a little help in creating the button styles. I know that the solution isn't perfect, especially the card correctly filling the screen. I'm a beginner and I'd like to know which tools and techniques are recommended for a beginner like myself for faster and better creation of the frontend.
@antarya
Posted
Hi,
Great start. It looks really good.
I have a couple of suggestions, though:
HTML
Wrap your content with the main tag. That way browser will know where to find primary content. Also, it will fix some accessibility issues. Here is a resource with examples related to semantic tags https://learn-the-web.algonquindesign.ca/topics/html-semantics-cheat-sheet/;
alt attribute should describe what is on the image. Screen reader users should have an idea of what is on the image based on the alt text.
Here are some resources:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/alt https://jakearchibald.com/2021/great-alt-text/
While your class names are pretty good, you might also want to know about another often used format called BEM. Here is a link to the BEM idea http://getbem.com/introduction/.
CSS
I noticed that you are using float to position elements of the plan. I think you can do it much easier using flexbox. Here are some resources: http://flexboxfroggy.com/ https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Flexible_Box_Layout https://css-tricks.com/snippets/css/a-guide-to-flexbox/ https://www.frontendmentor.io/resources under Interactive Tutorials
Try not to use fixed width and height, instead use alternatives min-width, max-width, min-height, max-height. That way, you do not restrict your element, and it can adapt to content.
For example, if you update text inside .summary-plan -> .plan-title and make it bigger, you will notice that at some point, the text will shift down and will be outside of the visual box. You can easily fix it by removing fixed height from .summary-plan and converting all to use flexbox. So, in this case, just removing would be enough.
For .summary-main instead of fixed width which will ruin your layout on screen less than < 370px you can use max-width: 370px; which will grow or shrink but will never be more than 370px;
In a real application, you do not control the size of the text; it will be dynamic. So it would be best if you create styles that can adapt to the content.
You use the br tag to separate different elements. You will have more control if you use padding/margin on elements.
Here is a good resource about margins that will be useful to know: https://www.joshwcomeau.com/css/rules-of-margin-collapse/
You may skip .hero-img border-radiuses if you add overflow: hidden; to .summary-main class.
You are using align-self, which do nothing in this case. This property is used with flex or grid. https://developer.mozilla.org/en-US/docs/Web/CSS/align-self
If you wonder how to remove extra space at the bottom of the image, apply display: block to the image.
I hope this will be helpful.
Keep up the good work! Cheers!
Marked as helpful
@filip65
Submitted
It was fun to build this game. I tought that it will take me more time to build, but I've done it in two days so i'm happy! 😁 Made my best to make it as good as possible. I also made it as PWA and it was my first time doing that. Any feedback would be hightly appreceated! ❤
@antarya
Posted
Hi Filip,
It is really good, and the animation is nicely done 👍
I would suggest a couple of refactoring/improvements that might clean the code a little bit and will point to the topic that might save you a day of debugging on another project.
Good to know:
In your code you have setScore(score + 1); which should work as expected in this case but it is a safer way to do it using functional updates setScore((prevScore) => prevScore + 1). In this case, you can remove score from props, and make it safer for async cases, which is very well explained in this article https://kentcdodds.com/blog/use-state-lazy-initialization-and-function-updates#dispatch-function-updates. Resource regarding functional updates https://reactjs.org/docs/hooks-reference.html#functional-updates.
You might also consider useful this post that compares custom prop ref and ref using forwardRef https://stackoverflow.com/questions/58578570/value-of-using-react-forwardref-vs-custom-ref-prop.
Refactoring/improvements:
In Choosing component you pass setIsChoosing which is used with handleUserChoice, why not to move setIsChoosing(false) to handleUserChoice?
In the Choosing component, if you update according to the previous suggestion, there is no need to have handleClick; you can directly pass handleUserChoice to Button.
Three-button in Choosing component looks like a good candidate to generate them on the fly:
const icons = {
paper: iconPaper,
scissors: iconScissors,
rock: iconRock,
};
const choices = ['paper', 'scissors', 'rock'];
function Choosing ... {
...
choices.map((choice) =>
<Button
image={icons[choice]}
type={choice}
handleClick={handleUserChoice}
text={`${choice} button`}
/>
)
...
}
Result component, which will make your code cleaner. // instead of this
image={
houseChoise === "paper"
? iconPaper
: houseChoise === "rock"
? iconRock
: iconScissors
}
// you can now write this
image={icons[houseChoise]}
For accessibility, add alt on the Rules close button.
Try to stick to one className format (score__text, house-btn, playAgainBtn) and do not use id for styling. Check this formatting rules http://getbem.com/introduction/.
I also noticed that when height is small, elements are on top of each other.
It is awesome that you added PWA support 🚀
I hope this will be helpful.
See you on the coding trail.
Marked as helpful
@nickcarlisi
Submitted
This is my second attempt at this challenge, implementing several small changes that were recommended when I submitted my first attempt. I added the box shadow on the 'proceed to payment' button (that I missed first time around) and added a 'main' tag for accessibility. I initially had an issue where the background image didn't seem to match the image provided. I figured out that I needed to add a background color AND the background image. My only issue now is that the margin around the "plan" section and the button seems too small. When I change the margin, nothing seems to happen. Any thoughts/suggestions? Thanks!
@antarya
Posted
Hi Nick,
Great start. It looks really good.
Regarding your question: you can fix it by removing the overflow: hidden; on the body tag. Also, use min-height instead of height to allow body to grow when needed. The current solution with overflow: hidden; disables the scrollbar when needed. Check the description of hidden https://developer.mozilla.org/en-US/docs/Web/CSS/overflow.
Also a couple of suggestions:
You may skip .hero-img border-radiuses if you add overflow: hidden; to .card class.
Check accessibility issues. It can be easily fixed by wrapping your content in the main tag. https://learn-the-web.algonquindesign.ca/topics/html-semantics-cheat-sheet/
Minor differences compared to design - the background has a missing bottom wave and no shadow on the button.
I hope this will be helpful.
Keep up the good work!
@Vanesa-R
Submitted
I want to improve my javascript with regexp... Recommendations for beginners?
@antarya
Posted
Hi Vanesa,
Great job, it looks really good.
I have a couple of suggestions/improvements, though:
There is an issue if you continue choosing tips the old selection stays active visually. You will easily fix that but as an alternative to type="button" you may also consider type="radio", as it is natively close to what you want to achieve for a group of buttons. Here is a link that might be helpful https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/radio.
It might not be the best idea to use id for calculation purposes. If you want to save data on elements you can use the data attribute e.g.: data-tip="5". Here is a link that might be helpful https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes
When you focus on empty custom input it shows NaN. You do not want users to see this so you may assign 0 while calculating if it is not valid or format output.
`$${(amount || 0).toFixed(2) }`;
For custom input field add default border to be transparent otherwise the whole block is jumping on focus.
It looks like you are using BEM (http://getbem.com/introduction/) rules. You have .--active modifier and you use it differently in each case. If you are not planning to have global .--active it might be better to be more specific like .tip--active.
As an alternative for type="text" you can use type="number" on the bill and people inputs which will give you some validation out of the box:
<input type="number" name="bill" id="bill" class="input__text" placeholder="0" step="50" min="0">
<input type="number" name="people" id="people" class="input__text" placeholder="0" step="1">
Here is a link that might be helpful https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number.
You might have an issue with input icons if the screen is too small or if the label will be longer. It will help to wrap input in a tag and add icon inside - for example how it is done here, inspect to see the structure https://material-ui.com/components/text-fields/#icons.
It would be nice to have some default values for tips (e.g. preselected 5%) and a number of persons (e.g. 1).
You may simplify/improve your code in some cases for example
// ---- 1
if (bill.value) {
bill.classList.add("--active");
} else {
bill.classList.remove("--active");
}
// instead you can use toggle
bill.classList.toggle("--active", bill.value);
// https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle
// ---- 2
people.classList.remove("--active");
people.classList.remove("--error");
// instead you can
people.classList.remove("--active", "--error");
// https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/remove
// ---- 3
const calc = () => {
// you calculate it only once
const valuePerPerson = valueBill / numberPeople;
amount = valuePerPerson * (tipSelected ? tipSelected : customTip)
totalTip = valuePerPerson + amount;
print();
}
print method is a good example of polluting the global scope which is a bad thing. The browser also has a print method that opens a print dialogue, but your version will be used instead. The simplest solution would be to wrap your whole script into (() => { ... your code })(); This is called Immediately-invoked Function Expressions (IIFE), you wrap your code with a function and then immediately call it. Here are links that might be helpful:
https://www.tutorialspoint.com/what-is-global-namespace-pollution-in-javascript
https://addyosmani.com/resources/essentialjsdesignpatterns/book/#detailnamespacing you can start from section 5. Immediately-invoked Function Expressions
Regarding regex you might check this post https://developer.mozilla.org/en-US/docs/Learn/Forms/Form_validation, they have a couple of examples of how to validate input. So you may try to validate input values to be numbered using regex.
I hope this will be helpful.
Keep up the good work!
Browser Version: 92.0.4515.107 (Official Build) (64-bit) OS: Ubuntu 20.04
@Faerk77
Submitted
Any feedback would be very appreciated.
@antarya
Posted
Hi,
Looks cool. Nice job.
I have a couple of suggestions, though:
HTML
div instead; your main tag should consist of the primary content of your page, e.g.:<main>
<div class="card">
...
</div>
</main>
alt attribute should describe what is on the image. Screen reader users should have an idea of what is on the image based on the alt text. In this case, you might leave it empty, which means the screen reader will skip it because you already have the person's name, and that should be enough. https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/altCSS
If you use reset styles, you will eliminate some differences across browsers or remove unnecessary spacing. You can read more about reset/normalize here https://css-tricks.com/reboot-resets-reasoning/. Also, it is helpful to include https://css-tricks.com/box-sizing/ in your reset.
.card-img can also be formated using BEM (http://getbem.com/introduction/) like .card__img or .card__avatar.
You will have trouble using fixed width and height for sections; you want all elements to adapt to viewport changes. So instead of fixed width/height on box1, .card__header, and card__body remove and add this:
.card {
...
width: 100%;
max-width: 348px; /* can be less but no more than 348px */
...
}
border-radius to the top section, so you used direct styles.
There is nothing wrong with applying border-radius to each corner, but there is a simpler solution. You need to add overflow: hidden; to the .card class and clean the rest..card { /* box1 in your case */
...
border-radius: 10px;
overflow: hidden;
...
}
One simpler solution to position avatar is instead of using bottom everywhere, remove it and use margin-top: -54px;, and clean unnecessary margins, and position: relative; from .card__header.
For avatar define fixed width and height, so it is predictable. In case you have a different image with no square dimensions, it will look strange.
.card__avatar {
display: block;
width: 100px;
height: 100px;
border: 4px solid #fff;
border-radius: 50%;
background: #fff;
/*
position: relative;
bottom: 52px;
*/
margin-top: -54px; /* shift image */
object-fit: cover; /* https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit */
}
/* assuming there is a reset */
html, body {
height: 100%;
}
main {
display: flex;
align-items: center;
justify-content: center;
margin: 0 auto; /* center main tag horizontally as we restricted width */
padding: 20px;
min-height: 100%; /* this will make sure main will at least occupy viewport height */
}
margin property https://developer.mozilla.org/en-US/docs/Web/CSS/margin /* top | right | bottom | left */
margin: 30px 0 0 0;
I hope this will be helpful.
Keep up the good work. Cheers!
Browser Version: 92.0.4515.107 (Official Build) (64-bit) OS: Ubuntu 20.04
Marked as helpful
@Eslam-mohamed7
Submitted
This is my first solution, I would like to receive any feedback and comments to improve my skills. Thanks.
@antarya
Posted
Hi Eslam,
I want to suggest a couple of improvements to your implementation.
These improvements are related to current issues:
HTML
main tag. That way browser will know where to find primary content.
Here is a resource with examples related to semantic tags https://learn-the-web.algonquindesign.ca/topics/html-semantics-cheat-sheet/;CSS
border-radius to each corner, but there is a simpler solution.
You can apply to the parent container and remove the rest of the border-radius related styles..card {
...
border-radius: 10px;
overflow: hidden;
...
}
Using position: absolute; will complicate things. Use it only if there is no other way. In this case, all can be done without. If you follow the below steps, you can remove all position: absolute; except image overlay.
From design, both sections look equal. This can be easily achieved by:
.card .content {
...
flex-basis: 50%;
...
}
.card .photo {
...
flex-basis: 50%;
...
}
position: absolute; related styles. And add this:html, body {
height: 100%;
}
main {
display: flex;
align-items: center; /* center vertically */
max-width: 1200px; /* this will restrict width of main tag to 1200px */
margin: 0 auto; /* center main tag horizontally as we restricted width */
padding: 20px;
min-height: 100%; /* this will make sure main will at least occupy viewport height */
}
.card .photo img {
/*
border-top-right-radius: 10px;
border-bottom-right-radius: 10px;
position: relative;
top: 1.1px;
*/
display: block; /* this will remove space in the bottom of the image */
width: 100%;
height: 100%;
object-fit: cover; /* this will stretch image to fit section */
}
Also, remove styles related to margin and width as it is not necessary anymore.
I hope this will be helpful.
Keep up the good work. Cheers!
Browser Version: 92.0.4515.107 (Official Build) (64-bit) OS: Ubuntu 20.04
Marked as helpful
@jiajinho
Submitted
This is my first guru solution, completed using React.js. There are still some issues regarding accessibility in the syntaxes, but I'll leave it for now.
Animations were done using react-spring and react-use-gesture.
The cactus SVG were created using Figma, most of the icons were sourced from Flaticon.
Feedbacks & criticisms are welcomed. :-)
@antarya
Posted
Looks gorgeous! Great Job.
I checked your code and have a couple of questions/suggestions:
Regarding commit messages, it looks like you are using some kind of versioning for each commit message. I never saw that approach. It is hard to understand what is that commit about unless you open a description. Why not use something like http://karma-runner.github.io/1.0/dev/git-commit-msg.html. And for semantic version https://semver.org/.
It might help navigate the application folder structure if you create a pages folder with page components.
Is there any reason why you use <Title as="h2" /> when it is already defined as styled.h2
It looks like TileLayerContext with tileLayers and FormContext is used only on specific pages? If yes, why not move them down to a specific page or get rid of context at all. You are using TileLayerContext in one place, and as far as I understand, it can be generated inside Locations.jsx outside of the component once.
Instead of the below code
const { isDesktop, isTablet } = useContext(MediaContext);
const imageURL = isDesktop ?
"url('/static/about/laptop/image-about-hero.jpg')" :
isTablet ?
"url('./static/about/tablet/image-about-hero.jpg')" :
"url('./static/about/mobile/image-about-hero.jpg')";
what do you think of this:
const { media } = useContext(MediaContext); // where media is laptop || tablet || mobile || or any other if you add more
const imageURL = `url('/static/about/${media}/image-about-hero.jpg')`;
This is what I noticed; other than that, it is really nicely done.
I hope this will be helpful. Cheers!
Marked as helpful
@DiegoCoriolano
Submitted
I added a new camp to view the total tip. Nice to people that have math's difficult. Difficult to work with JS because i am very noob. I can't did work the disable of the Reset button, but i will work it.
@antarya
Posted
Hi Diego,
Great job, it looks really good.
I have a couple of suggestions, though:
JS
getElementById each time you address a specific element.
Instead, you can assign it to variable and then reuse it. var billElement = document.getElementById("value-box");
var tip10 = document.getElementById("tip10");
// or in object
var tipElements = {
5: document.getElementById("tip5"),
...
50: document.getElementById("tip50"),
}
// and later in code to use as before
billElement.value;
tipElements.50.checked = false;
That way, your code will look cleaner and more performant because you do not ask to find a specific element every time you need it.
Based on that, you can write something like this.
const tipValues = [5, 10, 15, 25, 50];
function uncheckTips () {
tipValues.forEach(function (tip) {
tipElements[tip].checked=false;
})
}
disable effect, you can set the reset button's disable attribute to true and style it:input.reset[disabled] {
background-color: #b7b7b7;
}
CSS
tagName.className format instead of just class name .className? It looks redundant. label.label-radio
// and not
.label-radio
It is a bad practice to use id for styling; the reason is that you cannot reuse it later in your code. Id is used to identify elements uniquely.
In more complex applications, you may have another money input field. If you make it more reusable like .input-with-dollar, you can apply it to any input element. But in case input#value-box, you will not be able to reuse it.
Instead of div.container .info-calc you can write .info-calc, and nothing will change.
Question: were you trying to make a mobile-first approach?
If yes, your version might be incorrect. In your responsive file, you use @media screen and (max-width:400px){ which means you define styles for screens less than 400px which contradicts mobile-first approach.
You want to define all styles for the mobile version and shared styles in style.css and then in responsive file to overwrite those that are specific to a desktop using something like @media screen and (min-width:400px){.
There is an issue when the viewport width is between 400-640px, there is some part that becomes invisible, and there is no way to scroll horizontally. Changing the media max-width to 640px should fix this issue.
Try to stick to one naming format for class names, same for ids. (cantZero, tip-amount, tipamount-person, tip-result-value they are all different). To make your life easier, you may try to follow these rules BEM.
HTML
There is no need to use the title inside the body tag.
There is no need to define onclick on the label tag; you can do it on input. It will be still triggered as there is a relationship between input and label which you create by using the for attribute on the label tag.
I hope this will be helpful.
Keep up the good work. Cheers!
Browser Version: 92.0.4515.107 (Official Build) (64-bit) OS: Ubuntu 20.04
Marked as helpful
@jubeatt
Submitted
I have written all my process of this challenge in README.md. so you can check it on my Github. If you have any better way for this challenge or you found any wrong with it. Please give me some feedback here or send an email to me!
Thank you, guys!
@antarya
Posted
Looks cool. Nice job.
I have a couple of suggestions, though:
On the desktop, if you keep decreasing height, you will notice that because of overflow: hidden on section tag or height: 100vh; on body tag part of the section will be hidden and not scrollable. The solution depends on what you want to achieve. One of the solutions would be to change overflow: hidden; to overflow: auto; on section tag. So section tag will have a scrollbar. Otherwise, it would be best to restructure it slightly so the body is scrollable and the section tag is fully visible.
There are minor differences when compared to the original design. It looks excellent as it is, but if the designer sees it, they might be mad :). I will list them in case you do not notice.
The overall spacing between different strings do not match the one that is in design. Try to use a slider, and you will see the difference.
Second sentence of the "Gain access ..." paragraph is on the next line which might mean either designer wanted the second paragraph to be on the second line or paragraph width is limited using max-width.
From design second and third section header is bold.
The button text when hovered is hard to read; maybe making it darker instead of lighter will look better. (e.g. Material-UI Button )
There are a couple of accessibility issues that I think can be easily fixed. Wrap your content into the main tag. Use the h1 tag for the first header.
In toolkits like Bootstrap, the class name .row is used to wrap columns, so it is a little bit more abstract name. I guess renaming or using BEM (e.g. price-grid__row, price-grid__join, price-grid__subscription) will help to be more specific and will help in case other toolkits will overwrite it. One trick I use to find a proper name for class is to use https://www.thesaurus.com/ and find synonyms that better match.
It might be easier to define box-sizing in the reset file for all possible elements. You can check the example here CSS-Tricks box-sizing.
By the way, the structure and details of the Readme file are amazing.
Browser Version: 92.0.4515.107 (Official Build) (64-bit) OS: Ubuntu 20.04
I am new to this platform; if I missed or misunderstood something, I will appreciate the feedback.
Marked as helpful
