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

Select only forager and freegan types by default #467

Closed
ezwelty opened this issue Oct 2, 2024 · 8 comments · Fixed by #494
Closed

Select only forager and freegan types by default #467

ezwelty opened this issue Oct 2, 2024 · 8 comments · Fixed by #494
Assignees
Labels
enhancement New feature request good first issue Good for newcomers
Milestone

Comments

@ezwelty
Copy link
Collaborator

ezwelty commented Oct 2, 2024

To match user expectations and the behavior of the current interfaces, the new categories property of each location type should be used to initialise the type tree filter with only types whose categories property contains either forager or freegan. This should also be used as the default selection for deciding when to display the orange dot on the filter button in mobile view, which indicates that the current filter selection deviates from the default. Currently the default is that all types are selected.

This is an early precursor to full categories support, see #383.

@ezwelty ezwelty added enhancement New feature request good first issue Good for newcomers labels Oct 2, 2024
@frankgomezdev
Copy link
Contributor

Hey, I'd like to be assigned this issue to work on.

@ezwelty
Copy link
Collaborator Author

ezwelty commented Oct 3, 2024

@frankgomezdev Welcome! I assigned you this issue, feel free to ask @wbazant (who recently refactored the type tree filter) if you have trouble finding where to make this change in the code.

@frankgomezdev
Copy link
Contributor

Sorry, I think I misunderstood what was being asked in this issue and it may be way over my skill level. You can reassign this to someone else.

@frankgomezdev frankgomezdev removed their assignment Oct 8, 2024
@wbazant
Copy link
Collaborator

wbazant commented Oct 8, 2024

No problem! I'll try to provide some pointers, if you want to try to solve it you still can, or if anyone else wants to have a go they're welcome too.

The app fetches types from the backend, here is an API definition: https://petstore.swagger.io/?url=https://raw.githubusercontent.com/falling-fruit/api/main/docs/openapi.yml#/Types/get_types .

The default choice is populated after it loads in the fetchAndLocalizeTypes.fulfilled extraReducer here:

state.types = typesAccess.selectableTypes().map((t) => t.id)

That extraReducer is still a place to populate the default choice of types, but it should become something like typesAccess.selectableTypesWithCategories("forager", "freegan") or typesAccess.selectableTypesWithDefaultCategories() instead of typesAccess.selectableTypes(), so we need to add a new method. To do that, we have to do some work on the module which converts the types from the backend into the form used everywhere else, src/utils/localizedTypes.ts :

export type LocalizedType = {
LocalizedType needs to gain a field, categories: string[] and
export class TypesAccess {
TypesAccess needs the new method.

@frankgomezdev
Copy link
Contributor

frankgomezdev commented Oct 8, 2024

Ok, I think I was missing the first part with the API and the filterSlice.js file. I did add the field into the LocalizedType and created a method in the TypesAccess but wasn't seeing the changes. So is the expected output when this change is made to only have forager and freegan selected in the tree filter on the left on load?

Also, feel free to re-assign.

@wbazant
Copy link
Collaborator

wbazant commented Oct 8, 2024

To make sure you're putting in all the changes, you can follow the data step by step through the code base - in the browser's network tab you'll see the data fetched by getTypes , which is called in fetchAndLocalizeTypes in src/redux/typeSlice.js.

Types where a list of categories intersects with ['forager', 'freegan'], e.g a dumpster or apple tree, should be selected by default, but e.g an oak tree should be unselected by default.

@frankgomezdev
Copy link
Contributor

frankgomezdev commented Oct 8, 2024

Hey, back again!

I'm going to take a little break because I've been working on this for a few hours now but so far I have:

  • Added categories field to BaseType interface in apiSchema.ts
  • Updated LocalizedType interface to include categories field in localizedTypes.ts
  • Modified localize function to handle categories field
  • Ensured categories field is included in the typesAccessInLanguage function

I can follow the data and see the getTypes response using the browser's network tool and can confirm the categories property is there.

This is the new method I created for TypesAccess in the localizedTypes.ts file:

https://github.com/frankgomezdev/falling-fruit-web/blob/0b514a6f1f279f9e7978b3233a546dab7d2656c0/src/utils/localizedTypes.ts#L151-L157

The issue I'm having now is that when I update line 83 in filterSlice.js to something like:
state.types = typesAccess.selectableTypesWithCategories('forager', 'freegan')
All options are now unselected in the filter tree. Should I be working with the typeSlice.js file instead?

@wbazant
Copy link
Collaborator

wbazant commented Oct 9, 2024

Neat, looks like you did a lot on this!

The old code did

  [fetchAndLocalizeTypes.fulfilled]: (state, action) => {
      const typesAccess = action.payload
      state.types = typesAccess.selectableTypes().map((t) => t.id)
    },

so I'm guessing after you add .map((t) => t.id) to your proposed line, it will work.

Then you'll also have a small bit of logic to update in src/components/filter/FilterIconButton.js to determine whether a current choice is a default like Ethan asked in the issue description, and maybe check all usages of selectableTypes to see if that's all:

wbazant@wbazant-19-01:~/dev/falling-fruit-web$ ag selectableTypes
src/components/filter/FilterIconButton.js
13:    types?.length === typesAccess.selectableTypes().length &&

src/components/filter/Filter.js
85:                  typesAccess.selectableTypes().map((type) => type.id),

src/utils/localizedTypes.ts
96:  selectableTypes(): LocalizedType[] {

src/redux/filterSlice.js
83:      state.types = typesAccess.selectableTypes().map((t) => t.id)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants