-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Changes from all commits
df58082
5b02819
4073748
9797bf5
98a9cdc
9b5d637
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ import { | |
DefaultColumnFilter, | ||
SelectDifficultyColumnFilter, | ||
SelectColumnFilter, | ||
SelectCheckedColumnFilter, | ||
} from './filters'; | ||
import { Event } from '../Shared/Tracking'; | ||
|
||
|
@@ -31,20 +32,19 @@ import PatternFrequencies from '../PatternFrequencies'; | |
const iconPath = `${process.env.PUBLIC_URL}/assets/icons/`; | ||
|
||
const Table = () => { | ||
const data = React.useMemo(() => questions, []); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I remember correctly, we introduced 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
|
@@ -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( | ||
|
@@ -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}`}> | ||
|
@@ -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; | ||
|
@@ -194,11 +223,13 @@ const Table = () => { | |
] += additive; | ||
setDifficultyCount(difficultyCount); | ||
setChecked([...checked]); | ||
setData(filteredByCheckbox()); | ||
}} | ||
/> | ||
</span> | ||
); | ||
}, | ||
Filter: SelectCheckedColumnFilter, | ||
}, | ||
{ | ||
Header: 'Questions', | ||
|
@@ -384,6 +415,10 @@ const Table = () => { | |
defaultColumn, | ||
initialState: { | ||
filters: [ | ||
{ | ||
id: 'checkbox', | ||
value: localStorage.getItem('checkbox') || '', | ||
}, | ||
{ | ||
id: 'difficulty', | ||
value: localStorage.getItem('difficulty') || '', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,7 @@ | |
|
||
.reset-button { | ||
margin-top: 10px; | ||
margin-bottom: 10px; | ||
font-size: 0.7rem; | ||
} | ||
} |
There was a problem hiding this comment.
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? 🤔Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!