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

Add operatorId filter to ArrivalByTime widget #204

Merged
merged 8 commits into from
Nov 16, 2023
Merged

Conversation

Haswell-s
Copy link
Collaborator

@Haswell-s Haswell-s commented Nov 12, 2023

480p.mov

#148
Added a operatorId filter in Arrival widget

Haswell-s and others added 3 commits November 12, 2023 15:28
Make operatorSelector effect on ArrivalByTimeChart.
Change OperatorSelector to change to empty string if no operator selected, keeping it in line with the initial operatorId value.
Copy link

netlify bot commented Nov 12, 2023

Deploy Preview for astonishing-pothos-5f81bd ready!

Name Link
🔨 Latest commit 81083eb
🔍 Latest deploy log https://app.netlify.com/sites/astonishing-pothos-5f81bd/deploys/6555c604dda6060008306d7c
😎 Deploy Preview https://deploy-preview-204--astonishing-pothos-5f81bd.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

Nice! see suggestions

}) {
// Filter data if an operator is selected
if(operatorId !== ''){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(operatorId !== ''){
if(operatorId){

}) {
// Filter data if an operator is selected
if(operatorId !== ''){
data = data.filter( item => operatorId && item.id === operatorId);
Copy link
Member

Choose a reason for hiding this comment

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

consider useMemo

	const blabla = useMemo(()=>data.filter( item => !operatorId || item.id === operatorId), [data, operatorId])

@Haswell-s
Copy link
Collaborator Author

I updated the title too, it looks good but I could not verify. I couldn't get the text to update.

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

almost there!

src/locale/he.json Outdated Show resolved Hide resolved
@Haswell-s
Copy link
Collaborator Author

I did a --amend commit, droped the title one on force pushed it. Hope it didn't mess anything up

@NoamGaash
Copy link
Member

force push are allowed here, but there's no need to amend commits because we squash merge our PRs

Copy link
Member

@NoamGaash NoamGaash 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

}) {

const filteredData = useMemo(
() => data.filter((item) => operatorId && item.id === operatorId),
Copy link
Member

Choose a reason for hiding this comment

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

why not

Suggested change
() => data.filter((item) => operatorId && item.id === operatorId),
() => data.filter((item) => !operatorId || item.id === operatorId),

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Damn that's a smart one, haven't thought of that.
Can I still update the pr?

Copy link
Member

Choose a reason for hiding this comment

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

sure

Copy link
Member

Choose a reason for hiding this comment

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

in fact, you'll need to update it anyway as the linter is failing

@Haswell-s
Copy link
Collaborator Author

@NoamGaash I've tried to yarn lint before commiting and received this error. any advice?
Screenshot 2023-11-15 at 9 13 02

@NoamGaash
Copy link
Member

that's just a warning,
seems like the linter is still running on your machine
you can take a look at the linter output of the github runner - https://github.com/hasadna/open-bus-map-search/actions/runs/6859008868/job/18652201225?pr=204

Run yarn lint

/home/runner/work/open-bus-map-search/open-bus-map-search/src/pages/dashboard/ArrivalByTimeChart/ArrivalByTimeChart.tsx
Error:   [4](https://github.com/hasadna/open-bus-map-search/actions/runs/6859008868/job/18652201225?pr=204#step:5:5)2:1  error  Delete `⏎`  prettier/prettier

/home/runner/work/open-bus-map-search/open-bus-map-search/src/pages/dashboard/DashboardPage.tsx
Error:   16[5](https://github.com/hasadna/open-bus-map-search/actions/runs/6859008868/job/18652201225?pr=204#step:5:6):34  error  Replace `·data={convertToGraphCompatibleStruct(graphData)}·operatorId={operatorId}` with `⏎················data={convertToGraphCompatibleStruct(graphData)}⏎················operatorId={operatorId}⏎·············`  prettier/prettier

✖ 2 problems (2 errors, 0 warnings)
  2 errors and 0 warnings potentially fixable with the `--fix` option.

Error: Process completed with exit code 1.

@Haswell-s
Copy link
Collaborator Author

@NoamGaash Just know, what should I run before commiting?

@NoamGaash NoamGaash merged commit ca03c59 into hasadna:main Nov 16, 2023
11 checks passed
@NoamGaash
Copy link
Member

@i5x64BIT build, lint and test

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.

2 participants