Skip to content

implement filtering by completion status #180

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

Merged
merged 6 commits into from
Jul 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/components/Table/filters.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ function CreateDropDownListHelper(options, filterValue, setFilter, id) {
<select
value={filterValue}
onChange={e => {
setFilter(e.target.value || '');
localStorage.setItem(id, e.target.value);
setFilter(e.target.value || '');
Copy link
Owner

Choose a reason for hiding this comment

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

For my own learning, is this a best practice of updating localStorage first and then state? 🤔

Copy link
Contributor Author

@leo-step leo-step Jul 25, 2022

Choose a reason for hiding this comment

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

I swapped the order because the setFilter function for the checkbox filter reads localStorage and needs the value to be up to date. It doesn't make a difference for any of the other filters.

Copy link
Owner

Choose a reason for hiding this comment

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

This makes total sense to me - I forgot we had included this behaviour in #167!

}}
>
<option value="">All</option>
Expand Down Expand Up @@ -64,3 +64,15 @@ export function SelectColumnFilter({

return CreateDropDownListHelper(options, filterValue, setFilter, id);
}

export function SelectCheckedColumnFilter({
column: { filterValue, setFilter, id, filterByCheckbox },
}) {
const options = ['Checked', 'Unchecked'];
const filter = val => {
setFilter(val);
filterByCheckbox();
};

return CreateDropDownListHelper(options, filterValue, filter, id);
}
53 changes: 44 additions & 9 deletions src/components/Table/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
DefaultColumnFilter,
SelectDifficultyColumnFilter,
SelectColumnFilter,
SelectCheckedColumnFilter,
} from './filters';
import { Event } from '../Shared/Tracking';

Expand All @@ -31,20 +32,19 @@ import PatternFrequencies from '../PatternFrequencies';
const iconPath = `${process.env.PUBLIC_URL}/assets/icons/`;

const Table = () => {
const data = React.useMemo(() => questions, []);
Copy link
Owner

Choose a reason for hiding this comment

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

If I remember correctly, we introduced useMemo() to capture performance gains - see the docs here:

Pass a “create” function and an array of dependencies. useMemo will only recompute the memoized value when one of the dependencies has changed. This optimization helps to avoid expensive calculations on every render.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This new filter is a little different from the other ones because it works with a dynamic checkbox field instead of static properties like difficulty. Previously, the caching worked well because nothing could really be overwritten, but now I encountered problems with it because my assigned properties would constantly be reset. Using useState instead of useMemo solved this problem.

Also I'm not sure if there is a performance gain from useMemo for this line in particular. Isn't it just saving a reference to the questions array? It doesn't seem like a very expensive operation, but correct me if I'm wrong.

const [resetCount, setResetCount] = useState(0);
let checkedList =
JSON.parse(localStorage.getItem('checked')) ||
new Array(data.length).fill(false);
new Array(questions.length).fill(false);

/* If the user has previously visited the website, then an array in
LocalStorage would exist of a certain length which corresponds to which
questions they have/have not completed. In the event that we add new questions
to the list, then we would need to resize and copy the existing 'checked'
array before updating it in LocalStorage in order to transfer their saved
progress. */
if (checkedList.length !== data.length) {
const resizedCheckedList = new Array(data.length).fill(false);
if (checkedList.length !== questions.length) {
const resizedCheckedList = new Array(questions.length).fill(false);

for (let i = 0; i < checkedList.length; i += 1) {
resizedCheckedList[i] = checkedList[i];
Expand All @@ -54,13 +54,30 @@ const Table = () => {
window.localStorage.setItem('checked', JSON.stringify(checkedList));
}

const filteredByCheckbox = () => {
const checkbox = localStorage.getItem('checkbox') || '';
return questions.filter(question => {
if (!checkbox) return true;
return question.checkbox === checkbox;
});
};

for (let i = 0; i < questions.length; i += 1) {
if (checkedList[questions[i].id]) {
questions[i].checkbox = 'Checked';
} else {
questions[i].checkbox = 'Unchecked';
}
}

const difficultyMap = { Easy: 0, Medium: 0, Hard: 0 };
const totalDifficultyCount = { Easy: 0, Medium: 0, Hard: 0 };
for (let i = 0; i < data.length; i += 1) {
difficultyMap[data[i].difficulty] += checkedList[data[i].id];
totalDifficultyCount[data[i].difficulty] += 1;
for (let i = 0; i < questions.length; i += 1) {
difficultyMap[questions[i].difficulty] += checkedList[questions[i].id];
totalDifficultyCount[questions[i].difficulty] += 1;
}

const [data, setData] = useState(filteredByCheckbox());
const [difficultyCount, setDifficultyCount] = useState(difficultyMap);
const [checked, setChecked] = useState(checkedList);
const [showPatterns, setShowPatterns] = useState(
Expand Down Expand Up @@ -174,7 +191,12 @@ const Table = () => {
</span>
);
},
id: 'Checkbox',
accessor: 'checkbox',
id: 'checkbox',
filterByCheckbox: () => {
setData(filteredByCheckbox());
},
disableSortBy: true,
Cell: cellInfo => {
return (
<span data-tip={`Question #${Number(cellInfo.row.id) + 1}`}>
Expand All @@ -185,7 +207,14 @@ const Table = () => {
checked[cellInfo.row.original.id] = !checked[
cellInfo.row.original.id
];

const question = questions.find(
q => q.id === cellInfo.row.original.id,
);
if (checked[cellInfo.row.original.id]) {
question.checkbox = 'Checked';
} else {
question.checkbox = 'Unchecked';
}
const additive = checked[cellInfo.row.original.id]
? 1
: -1;
Expand All @@ -194,11 +223,13 @@ const Table = () => {
] += additive;
setDifficultyCount(difficultyCount);
setChecked([...checked]);
setData(filteredByCheckbox());
}}
/>
</span>
);
},
Filter: SelectCheckedColumnFilter,
},
{
Header: 'Questions',
Expand Down Expand Up @@ -384,6 +415,10 @@ const Table = () => {
defaultColumn,
initialState: {
filters: [
{
id: 'checkbox',
value: localStorage.getItem('checkbox') || '',
},
{
id: 'difficulty',
value: localStorage.getItem('difficulty') || '',
Expand Down
1 change: 1 addition & 0 deletions src/components/Table/styles.scss
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@

.reset-button {
margin-top: 10px;
margin-bottom: 10px;
font-size: 0.7rem;
}
}