Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add links in the homepage #11

Merged
merged 12 commits into from
Feb 24, 2022
Merged

feat: add links in the homepage #11

merged 12 commits into from
Feb 24, 2022

Conversation

CarmitKl
Copy link
Contributor

@CarmitKl CarmitKl commented Feb 22, 2022

Add 6 cards to the Homepage, which on-click navigate to pages in the website. Adds material-ui dependency to the project.
Defined to have 3 columns for medium+ screens, so It's preferable to have at least 3 cards (it can be any number).

Cards are defined by the file: src > config > home.json.

… website. this adds material-ui to the project
@CarmitKl CarmitKl requested a review from dan-ziv February 22, 2022 10:59
import {useHistory} from '@docusaurus/router';

export default function HomepageFeatures() {
const list = homeLinksList;
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant assignment

<Button
variant="contained"
onClick={() => {
console.log('linkName: ', linkName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider moving event handlers outside and maybe even use useCallback

history.push(linkName);
}
}}
sx={{
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider moving style objects outside the JSX


const HomepageCard = ({title, text, img}) => {
console.log('got img: ', img);
const blue = '#29296E';
Copy link
Collaborator

Choose a reason for hiding this comment

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

export this color from css file?

}
};

const renderCards = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move renderCards inside the HomepageFeatures component

onKeyDown={e => handleOnKeyDown(e, linkName)}
sx={overrideLinkStyle}
>
<HomepageCard {...props}></HomepageCard>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<HomepageCard {...props}></HomepageCard>
<HomepageCard {...props}/>

: null;
};

const HomepageFeatures = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const HomepageFeatures = () => {
export const HomepageFeatures = () => {

import {CardContent, CardMedia, Typography} from '@mui/material';
import {Blue} from '../../config/colors';

const HomepageCard = ({title, text, img}) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const HomepageCard = ({title, text, img}) => {
export const HomepageCard = ({title, text, img}) => {

@@ -0,0 +1,9 @@
export const Blue = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not in css file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'colors' should be reused between many files. Overriding mui-components is much easier using the 'sx' prop, than creating a ccs and importing it, and making sure in it to override relevant classes as seen on the browser (need to manually search the dom).

…t only with 'default export'; move back hook 'useHistory' to the top of the component;
);
};

export default HomepageCard;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

);
};

export default HomepageFeatures;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

…t', leave only named export for the Homepage-comps
@CarmitKl CarmitKl merged commit cb7347c into dev Feb 24, 2022
@CarmitKl CarmitKl deleted the feat/add-mui-homepageLinks branch February 24, 2022 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants