Skip to content

Conversation

@mvolkmann
Copy link
Collaborator

No description provided.

S78901
S78901 previously approved these changes Apr 26, 2024
<input
id="all-skills"
type="checkbox"
value="false"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are using the checked property now instead of the value property.

<input
id="all-skills"
type="checkbox"
value="false"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are using the checked property now instead of the value property.

@@ -1,10 +1,11 @@
import React, { useContext, useState } from 'react';
import { Link } from 'react-router-dom';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was just moved up a few lines.

<label htmlFor="all-skills">Show all skills</label>
<input
onClick={handleClick}
onChange={handleClick}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's recommended to use onChange with checkboxes instead of onClick.

@@ -1,38 +1,37 @@
import React, { useCallback, useContext, useEffect, useState } from 'react';
import { styled } from '@mui/material/styles';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reordered several of the imports.

const [query, setQuery] = useState('');
const [skillFilter, setSkillFilter] = useState(null);

useQueryParameters([
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This adds support for maintaining state with query parameters.


const handleClick = () => setShowAllSkills(!showAllSkills);

useQueryParameters([
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This adds support for maintaining state with query parameters.

@mvolkmann mvolkmann requested a review from S78901 April 29, 2024 19:23
Copy link
Contributor

@S78901 S78901 left a comment

Choose a reason for hiding this comment

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

Looks good!

@mvolkmann mvolkmann merged commit d0058f8 into develop Apr 29, 2024
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.

4 participants