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

feat: filter url parameters #996

Closed

Conversation

snturk
Copy link

@snturk snturk commented Feb 14, 2023

Description

discord username: Tundra#9805

**closes #893 **

Filter can now be able to get relevant URL parameters like 'lastTime=last30Minutes' and when a filter change, a parameter added to url to share the link.

## Things to check before creating a PR

  • I have inspected my topic, checked.
  • If it is a core feature, executed detailed tests.

Creating PR rules

  • PR must be created for an issue with approved tag. Otherwise PR will be rejected.
  • Relevant issue number: The issue number related to PR must be attached to head of PR header, after prefix after prefix # must be attached in parenthesis. A header like this could be used "prefix(#issue_number): PR header" .
  • A descriptive and understandable title: The PR title should clearly describe the nature and purpose of the changes. The PR title should be the first thing displayed when the PR is opened. And it should follow the semantic commit rules. For example, a title like "docs(#issueId): Add README.md" can be used.
  • Related file selection: Only relevant files should be touched and no other files should be affected.
  • Format and Lint Suitability : The code should be made in accordance with a certain format standard and examined according to the lint rules.
  • Clean commit history: The commits where the changes are made should be clear and organized.
  • Opening PR when job is completed: PR should be opened when job is completed and sent for review by other team members.

Changes

  • Filter related components (ChannelFilterMenu, CommonFilterMenu, FilterTimeMenu, ReasonFilterMenu) improved with url parameters
  • addParameterToUrl and getParameterValueFromUrl util functions added

How were these changes tested?

I've tried various combinations of URL parameters, no problem.

Example Link: LOCALHOST/en?lat=38.094574707566274&lng=35.211181640625&zoom=7&lastTime=last3Hours&reasons=erzak%2Cyemek&channel=twitter

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

@snturk snturk requested a review from a team February 14, 2023 21:10
@vercel
Copy link

vercel bot commented Feb 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
deprem-yardim-frontend ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 16, 2023 at 3:53PM (UTC)

src/components/UI/FilterMenu/CommonFilterMenu.tsx Outdated Show resolved Hide resolved
@@ -29,6 +30,7 @@ export const ReasonFilterMenu: React.FC = () => {
const [filterValues, setValues] = React.useState<string[]>(
reasonFilterMenuOptions
);
const parameterName = "reasons";
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here as above

Copy link
Author

Choose a reason for hiding this comment

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

Didn't understand :( Is there a problem related to line 33?

src/utils/helpers.ts Outdated Show resolved Hide resolved
@eraygundogmus
Copy link
Collaborator

Meanwhile, I can see your deployment has failed. To fix it

  • yarn build
  • find errors
  • fix
  • commit and push

@snturk snturk force-pushed the feature/893/filter_url_param branch from 202e11d to a3cb348 Compare February 15, 2023 05:44
@snturk
Copy link
Author

snturk commented Feb 15, 2023

Meanwhile, I can see your deployment has failed. To fix it

  • yarn build
  • find errors
  • fix
  • commit and push

Done, no errors occurred.

efegurkan
efegurkan previously approved these changes Feb 15, 2023
Copy link
Contributor

@efegurkan efegurkan left a comment

Choose a reason for hiding this comment

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

Seems good to me after the changes. Someone else also please take a look.

@efegurkan efegurkan self-requested a review February 15, 2023 09:45
Copy link
Contributor

@efegurkan efegurkan left a comment

Choose a reason for hiding this comment

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

Seems good, in general. Thank you for your contribution <3

There is one case that I figured after the deployment.

  • If we select a few reason filters they are added to URL as expected.
  • At first refresh filters are set on page load as expected.
  • Once page load is done, even though filters are set. They are not on the url anymore.

I tried with following
https://deprem-yardim-frontend-dd8asetrr-afet.vercel.app/en?lat=37.535557424752696&lng=36.89483642578126&zoom=8&reasons=erzak%2Chayvanlar-icin-tedavi%2Ckonaklama%2Ckurtarma%2Clojistik%2Csu%2Cyagma%2Cyemek

If you refresh page twice, filters are gone from url.
https://deprem-yardim-frontend-dd8asetrr-afet.vercel.app/en?lat=37.535557424752696&lng=36.89483642578126&zoom=8

I revoked my approval, sorry for any inconveniences I made.

Deployment seems to have issues with translations but I don't think it is related to this PR 🤔

@snturk
Copy link
Author

snturk commented Feb 15, 2023

Some other params like lat or zoom was overriding the existing ones. So commit 17b4bc7 fixes that issue with keeping the existing ones.

@usirin
Copy link
Contributor

usirin commented Feb 19, 2023

we are doing a big refactor & redesign ty so much for the contribution and sorry for not taking this in.

@usirin usirin closed this Feb 19, 2023
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.

bug: Filters are not kept when user refreshes the page
4 participants