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

Alphonso's review #33

Open
camelPhonso opened this issue Oct 25, 2023 · 1 comment
Open

Alphonso's review #33

camelPhonso opened this issue Oct 25, 2023 · 1 comment

Comments

@camelPhonso
Copy link

  • love the idea! made me chuckle
  • looks like you might be over-complicating really simple issues. Here's one:
// in FilterAndDisplay.jsx
<Select
	className="custom-select"
	classNames={{
	control: (state) =>
		state.isFocused ?    // this is cool
		'border-red-600' :   // but why have it??
		'border-grey-300', 
	}}
	options={options}
	value={selectedOption}
	onChange={setSelectedOption}
>		
</Select>
/* why not this in your global.css ?? */
.custom-select {
	border-color: rgb(75 85 99); /* this matches the tailwind class */
}

.custom-select:active {
	border-color: rgb(220 38 38);
}
  • this two-part filter is awesome! ![[Screenshot 2023-10-25 at 17.29.44.png]]
  • why does your RenderServants component not simply display the filter conditionally, instead of living in a different route?
    • the power of React is in building "single page apps"!
const [max, setMax] = useState(Math.ceil(Math.max(...allServants)))
	// I literally copied the formula from an old project
	// I don't remember how it works

	// but now your max is the highest price you have available

	// you can just display everything with the filter in one page
@camelPhonso
Copy link
Author

lol, I don't know how to post screenshots 😆

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

No branches or pull requests

1 participant