Looking at the code …
Don’t put all the React code in one file. Break it up into files. Show them that you know how to organize a project. Yeah, you can get away with that on something small, but that won’t scale for an app with 100k lines and hundreds of components. Every React component should its own file, probably in its own folder grouped with its support files.
I get nervous when I see a prop called “state”. Just pass what you need.
const [state, dispatch] = useReducer(reducer, initialState);
const whoExpanded = state.whExpanded;
In general, “state” is a horrible variable name, unless you are in an actual reducer, etc. Use destructuring or a selector:
const [{whExpanded: whoExpanded}, dispatch] = useReducer(reducer, initialState);
And if you call the property on state as “whoExpanded”, it gets simpler - and more readable.
const About = () => {
const [text, setText] = useState(about);
return (
<div>
<ReactMarkdown children={text} />
</div>
)
}
Does this need state? That “about” should be imported in this file, where it is needed. Do you need that div?
const checkHide = (val) => {
if (val === whoExpanded) {
return "";
} else if (whoExpanded === "all") {
return "";
} else {
return " hidden";
}
}
First, “val” is a terrible variable name unless you really have know idea what it will be. And this is simplified and more readable as:
const checkHide = (category) =>
[whoExpanded, 'all'].includes(category)
? ''
: 'hidden'
const ExpBank = (props) => {
const [state, dispatch] = [props.state, props.dispatch];
return experienceTexts && experienceTexts.map((text, index) => {
return (
<ExpFrame state={state} dispatch={dispatch} key={index} text={text} name={expFolders[index]} />
)
})
}
What about:
const ExpBank = ({state, dispatch}) => {
if (!experienceTexts) {
return null
return experienceTexts.map((text, index) => {
return (
<ExpFrame state={state} dispatch={dispatch} key={index} text={text} name={expFolders[index]} />
)
})
}
or something like that.
Actually:
const ExpBank = ({state, dispatch}) =>
experienceTexts?.map((text, index) => (
<ExpFrame state={state} dispatch={dispatch} key={index} text={text} name={expFolders[index]} />
)
might work, too.
function checkExpand(who, state, dispatch) {
if (state.whExpanded === "all") {
dispatch({ type: UEXP, payload: who });
} else {
dispatch({ type: UEXP, payload: "all" });
}
}
might be:
function checkExpand(who, state, dispatch) {
dispatch({ type: UEXP, payload: state.whExpanded === "all" ? who : "all" });
}
or if you want to make it a little more readable:
function checkExpand(who, { whExpanded }, dispatch) {
const payload = whExpanded === "all" ? who : "all";
dispatch({ type: UEXP, payload);
}
onClick={() => changeNM()}
in this case would be the same as:
onClick={ changeNM }
Also, call it “changeNightMode” or whatever - we don’t get charged by the letter. Clarity is much, much, much more important - don’t make them guess or assume.
if (isOn) {
whiteToBlack("white");
} else {
whiteToBlack("black");
}
should be:
whiteToBlack(isOn ? "white" : "black");
function inv(color) {
if (color === "white") {
return "black";
} else if (color === "black") {
return "white";
} else {
return 0xffffff ^ color;
}
}
should be:
function inv(color) {
if (color === "white") {
return "black";
}
if (color === "black") {
return "white";
}
return 0xffffff ^ color;
}
avoid nesting and chaining where you can.
document.documentElement.style.setProperty('--whitefornight', revColor);
document.documentElement.style.setProperty('--blackfornight', color);
Don’t manipulate the DOM directly in React. Only React messes with the DOM, period. If you find yourself referencing document (other than getting your main wrapper element in your root index.js), then you are probably doing something un-React-y.
const styles = [
{
// ...
Don’t store those in an array. If you must do it this way, store it in an object you can give each of those style objects a meaningful name.
const reducer = (state, action) => {
const value = action.payload;
switch (action.type) {
case UEXP:
return { ...state, whExpanded: value }
break;
case UNMD:
return { ...state, nightMode: value }
break;
default:
return { ...state }
break;
}
}
If you’re returning a value in a case, you don’t need break.
Also:
default:
return { ...state }
Why return a copy of state? Since you are not changing it, just return state.
In general, look at some other React projects to see how they are organized, in terms of file and folder structures. There is more than one way to do it, but there are some basic principles.
Use standard formatting. What you have isn’t bad, but it would be good to install something like eslint and use some standard format - it’s what they are used to seeing. Something like prettier is nice, too.
Sorry if that comes off as harsh - I’m sure my projects at this stage where ripe for the same nitpicking. But those are the things that I notice as an experienced dev. If I were on the hiring committee, those are the question that I would ask.
Keep it up, you’ll get there.
