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

APP-323 Integrate project filters #2539

Merged
merged 21 commits into from
Nov 15, 2024
Merged

Conversation

wgardiner
Copy link
Contributor

@wgardiner wgardiner commented Nov 11, 2024

Description

Closes: APP-323


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. Navigate to all projects list page, review filters behavior on mobile and desktop

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 59c68ae
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/6737530d990f5600089a2df8
😎 Deploy Preview https://deploy-preview-2539--regen-website.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.

@wgardiner wgardiner force-pushed the feat-APP-323-project-filters-integration branch from 5bd7e8c to e980e74 Compare November 11, 2024 12:35
@wgardiner wgardiner changed the title integrate project filters APP-323 Integrate project filters Nov 11, 2024
Copy link

netlify bot commented Nov 13, 2024

Deploy Preview for terrasos failed. Why did it fail? →

Name Link
🔨 Latest commit d521699
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/67374643cf55470008b87a47

@wgardiner wgardiner force-pushed the feat-APP-323-project-filters-integration branch 2 times, most recently from 95d0b36 to 9a06190 Compare November 13, 2024 10:51
@wgardiner wgardiner marked this pull request as ready for review November 13, 2024 10:51
@wgardiner wgardiner requested review from blushi and flagrede November 13, 2024 10:51
@wgardiner wgardiner force-pushed the feat-APP-323-project-filters-integration branch from 9a06190 to 18c48c0 Compare November 13, 2024 11:45
@blushi
Copy link
Member

blushi commented Nov 13, 2024

could you run i18n:extract again?
this makes the new strings look like this

image

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

  • I don't think we should be able to have both the left side bar and the mobile filter modal open on desktop and the filter sidebar is still there on mobile
image
  • If I use this branch with regen client (not terrasos), I get a mix of both existing regen and terrasos filters:
image
  • Also nothing happens if I click on the region or ecosystem filter items.

I still see several TODOs and console.log's, let me know if I can help to finish that up asap.

@S4mmyb
Copy link
Member

S4mmyb commented Nov 13, 2024

Hey @wgardiner et all , some things I noticed:

  • Noticing this is missing the button outline when you click a filter button. Right now all that happens is the color changes on the region from yellow to gray, but there's no outline on the button as well which makes it confusing / hard to know what is selected and what's not selected.
  • Like @blushi said, we shouldn't have both the sidebar filter and the modal in desktop. The designs originally showed that we only had the sidebar. That said, I'm noticing when I try and do split screen and the sidebar disapears it's nice to have the top modal. So, I think the desktop behavior should be that the mobile filter modal doesn't show up until the window size is too narrow to show the sidebar filter, at which point the sidebar filter is removed and the mobile filter modal appears. Thoughts @erikalogie @blushi ?
  • On mobile when I select a filter, I can't unselect it.
  • The text "add filters" and "edit filters" are longer in spanish ("Agregar filtros" and "Editar filtros"). What's the feature behavior for that when the window is smaller. Will we do two lines to display that? @wgardiner if you could add in some real text values on the deploy preview to test these that would be great. In general it's confusing to have deploy previews without the real text.
  • When we just select the "compliance" filter, how do we display projects which are tagged as both compliance and voluntary? Right now when it filters, we show the tebu logo instead of the ha logo. Is that what we want to happen? Or when they select only compliance, do we show the ha logo with the size? cc @erikalogie
  • Right now the button to view more filters cuts off in the middle of one of the filters. In the designs we show that it should show x amount of filter rows then the see more filters button (see photo below)
  • If I unselect both the voluntary and compliance filters, I am still able to see a project. Is this what we expect to happen? Shouldn't I not be able to see any of the projects? cc @erikalogie

image

@S4mmyb
Copy link
Member

S4mmyb commented Nov 13, 2024

Re-finishing this up. @wgardiner seems like there's still a bunch to do here so if @blushi can take on contributing to tie this up just let us know as we have a day left on the deliverable. We can also have @r41ph review

@blushi
Copy link
Member

blushi commented Nov 13, 2024

  • When we just select the "compliance" filter, how do we display projects which are tagged as both compliance and voluntary? Right now when it filters, we show the tebu logo instead of the ha logo. Is that what we want to happen? Or when they select only compliance, do we show the ha logo with the size? cc @erikalogie

@S4mmyb per this dev note on figma: https://www.figma.com/design/brkxGV5qNOkZUp0YQl1cxO/Terrasos-Phase-1?node-id=1111-319415&node-type=instance&t=8N51My8XTlyuHu3w-0 if a project is both compliance and voluntary, then we show the tebu tag

@wgardiner
Copy link
Contributor Author

wgardiner commented Nov 14, 2024

I just pushed updates

  • added region filter support
  • reran i18n:extract
  • fixed desktop/mobile behavior for hiding mobile version of filters
  • fixed layout for Regen (hiding new filters)
  • added outline to selected filter tags
  • fixed "reset filters" text button that appears in card when no projects are shown

For more discussion

  1. unselecting filters on mobile (this works for me, @S4mmyb maybe you can provide more info)
  2. When we just select the "compliance" filter, how do we display projects which are tagged as both compliance and voluntary?

I'm not sure about the ha/tebu badge behavior, I haven't touched that in this PR.

  1. If I unselect both the voluntary and compliance filters, I am still able to see a project. Is this what we expect to happen? Shouldn't I not be able to see any of the projects?

This is what I was talking about in slack with filter intersection versus union. Currently we're displaying projects that satisfy any of the filter conditions, across all filter types. So for example, a project will still display even if no market type is selected, if it satisfies another condition, e.g. an environment type or region. We could choose to change this to only show projects that satisfy any of the filter conditions, but only within each category, so deselecting both market types would hide all projects.

@wgardiner wgardiner force-pushed the feat-APP-323-project-filters-integration branch from 26d4f1d to 104187c Compare November 14, 2024 10:18
@wgardiner
Copy link
Contributor Author

@blushi do you have an idea why I'd be having issues loading the icons from sanity for ecosystem types?

I double checked the keys that I'm using and they all seem to match what's in sanity, but I'm only getting back URLs for two of them. Here's what I'm getting from useEcosystemTags here

export const useEcosystemTags = (ecosystemTypes: string[]) => {
const [selectedLanguage] = useAtom(selectedLanguageAtom);
const { data: allProjectEcosystemData } = useQuery(
getAllEcosystemQuery({
sanityClient,
languageCode: selectedLanguage,
}),
);
const projectEcosystemIconsMapping = getIconsMapping({
data: allProjectEcosystemData?.allProjectEcosystem,
});
const ecosystemTags = Object.fromEntries(
ecosystemTypes?.map(ecosystem => [
ecosystem,
projectEcosystemIconsMapping?.[ecosystem.toLowerCase()] ?? '',
]) ?? [],
);
return ecosystemTags;

{
  "Low Montane Very Humid Forest": "",
  "Premontane Humid Forest": "https://cdn.sanity.io/images/jm12rn9t/staging/0c4e63baa70ba99b90c95d23b398bc0668cd1aec-50x50.svg",
  "Tropical Dry Forest": "https://cdn.sanity.io/images/jm12rn9t/staging/54dbff40009c4d2d7391b268ee714bfbec06896d-50x50.svg",
  "Tropical Humid Forest": "",
  "Tropical Very Humid Forest": ""
}

@blushi
Copy link
Member

blushi commented Nov 14, 2024

@wgardiner "Low Montane Very Humid Forest" doesn't seem to be on neither staging nor prod sanity dataset (we use staging on deploy previews FYI) @erikalogie could you add it along with an icon?

I need to debug to see what's wrong with the other 2

@blushi
Copy link
Member

blushi commented Nov 14, 2024

@wgardiner "Low Montane Very Humid Forest" doesn't seem to be on neither staging nor prod sanity dataset (we use staging on deploy previews FYI) @erikalogie could you add it along with an icon?

I need to debug to see what's wrong with the other 2

This had to do with the language not being set on the sanity documents. Btw this logic of having hardcoded ecosystem types/regions is not gonna work well with i18n.

@erikalogie
Copy link
Collaborator

@wgardiner "Low Montane Very Humid Forest" doesn't seem to be on neither staging nor prod sanity dataset (we use staging on deploy previews FYI) @erikalogie could you add it along with an icon?

Fixed

@erikalogie
Copy link
Collaborator

FYI @wgardiner the styling of the buttons is off:

Monosnap Terrasos Phase 1 2024-11-14 08-40-30

@blushi blushi force-pushed the feat-APP-323-project-filters-integration branch from 7fdffba to c7ac163 Compare November 14, 2024 14:41
@blushi
Copy link
Member

blushi commented Nov 14, 2024

@erikalogie @S4mmyb please have another look
Just be aware that on staging, our projects metadata is a bit incomplete, some are missing region or ecosystemType data so some projects might never appear when you have the filters on, I'd rather focus on completing some remaining dev tasks rather than adding those now.

@S4mmyb
Copy link
Member

S4mmyb commented Nov 14, 2024

@wgardiner "Low Montane Very Humid Forest" doesn't seem to be on neither staging nor prod sanity dataset (we use staging on deploy previews FYI) @erikalogie could you add it along with an icon?

Fixed

I see logos on desktop but not mobile

@S4mmyb
Copy link
Member

S4mmyb commented Nov 14, 2024

@erikalogie @S4mmyb please have another look Just be aware that on staging, our projects metadata is a bit incomplete, some are missing region or ecosystemType data so some projects might never appear when you have the filters on, I'd rather focus on completing some remaining dev tasks rather than adding those now.

Hey @blushi , this looks good to me! All of my comments and I believe @erikalogie's were addressed. The only thing I was noticing is that on mobile the ecosystem type logos and projects weren't displaying, but on desktop they were. I'm assuming though, this relates to this comment. If so I think this looks good

@erikalogie
Copy link
Collaborator

The spacing on the no results looks weird:
Screenshot 2024-11-14 at 11 31 53 AM

I also feel the drop shadow on the filter buttons looks a bit heavier than in the designs.

Ideally the ecosystem filter tags would fit two on one line, it looks a bit weird how it is. So either by including less padding on the right, or making the filters a bit wider so two appear per line.

Also, I think that in an ideal world, the filters that no longer apply would disappear once you start clicking on others. For example, if the user clicks "caribbean" then only the ecosystems that apply to caribbean projects would appear. I know that wasn't part of the specs but maybe should be a separate task we tackle early next week so there aren't so many zero results.

@blushi
Copy link
Member

blushi commented Nov 14, 2024

I also feel the drop shadow on the filter buttons looks a bit heavier than in the designs.

This was my impression too, though I've just copied/pasted what's on figma

@blushi
Copy link
Member

blushi commented Nov 14, 2024

Ideally the ecosystem filter tags would fit two on one line, it looks a bit weird how it is. So either by including less padding on the right, or making the filters a bit wider so two appear per line.

Looking at your screenshot the ecosystem icons don't appear for you some reason. Which browser do you use? Maybe you have cross site requests disabled.

image

@blushi blushi force-pushed the feat-APP-323-project-filters-integration branch from 7150159 to cf4efa1 Compare November 14, 2024 15:56
@blushi
Copy link
Member

blushi commented Nov 14, 2024

@erikalogie what do you think about this for having 2 ecosystem per line? i've made the filter sidebar larger and lowered down the tag max width. One drawback is that this results in some ecosystem types being displayed on 3 lines since they are quite long. Let me know what you think.

image

@blushi
Copy link
Member

blushi commented Nov 14, 2024

Also, I think that in an ideal world, the filters that no longer apply would disappear once you start clicking on others. For example, if the user clicks "caribbean" then only the ecosystems that apply to caribbean projects would appear. I know that wasn't part of the specs but maybe should be a separate task we tackle early next week so there aren't so many zero results.

FYI the many zero results here are really because of the staging data being incomplete as I said above. But this could be an improvement for sure. We just need to see where to draw the line between v1 and v2, otherwise the scope is ever expanding.

@erikalogie
Copy link
Collaborator

Ideally the ecosystem filter tags would fit two on one line, it looks a bit weird how it is. So either by including less padding on the right, or making the filters a bit wider so two appear per line.

Looking at your screenshot the ecosystem icons don't appear for you some reason. Which browser do you use? Maybe you have cross site requests disabled.

image

I'm using Brave. I just took down the shields for Brave and I still don't see them, but I remember seeing them on a different deploy preview.

@erikalogie
Copy link
Collaborator

@erikalogie what do you think about this for having 2 ecosystem per line? i've made the filter sidebar larger and lowered down the tag max width. One drawback is that this results in some ecosystem types being displayed on 3 lines since they are quite long. Let me know what you think.

image

This looks weird, I don't like it wider. Let's just leave it how it was for now.

@erikalogie
Copy link
Collaborator

I also feel the drop shadow on the filter buttons looks a bit heavier than in the designs.

This was my impression too, though I've just copied/pasted what's on figma

Yeah I'm sure you did, maybe just tweak it a little bit to make it look more like the Figma if possible

@erikalogie
Copy link
Collaborator

Also, I think that in an ideal world, the filters that no longer apply would disappear once you start clicking on others. For example, if the user clicks "caribbean" then only the ecosystems that apply to caribbean projects would appear. I know that wasn't part of the specs but maybe should be a separate task we tackle early next week so there aren't so many zero results.

FYI the many zero results here are really because of the staging data being incomplete as I said above. But this could be an improvement for sure. We just need to see where to draw the line between v1 and v2, otherwise the scope is ever expanding.

Yeah, I'll make a task in the v2 epic

@S4mmyb
Copy link
Member

S4mmyb commented Nov 14, 2024

@erikalogie what do you think about this for having 2 ecosystem per line? i've made the filter sidebar larger and lowered down the tag max width. One drawback is that this results in some ecosystem types being displayed on 3 lines since they are quite long. Let me know what you think.
image

This looks weird, I don't like it wider. Let's just leave it how it was for now.

Agreed - I think if its wider we cant do 3-wide projects either

@blushi
Copy link
Member

blushi commented Nov 14, 2024

Ideally the ecosystem filter tags would fit two on one line, it looks a bit weird how it is. So either by including less padding on the right, or making the filters a bit wider so two appear per line.

Looking at your screenshot the ecosystem icons don't appear for you some reason. Which browser do you use? Maybe you have cross site requests disabled.
image

I'm using Brave. I just took down the shields for Brave and I still don't see them, but I remember seeing them on a different deploy preview.

This is working for me on Brave too. Maybe clear your browser cache and reload?

@blushi
Copy link
Member

blushi commented Nov 14, 2024

I also feel the drop shadow on the filter buttons looks a bit heavier than in the designs.

This was my impression too, though I've just copied/pasted what's on figma

Yeah I'm sure you did, maybe just tweak it a little bit to make it look more like the Figma if possible

Updated the box shadow from 2px to 1px, please have a look @erikalogie

Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for terrasos ready!

Name Link
🔨 Latest commit 59c68ae
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/6737530d7ac2cf00081924bc
😎 Deploy Preview https://deploy-preview-2539--terrasos.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.

@blushi blushi merged commit 5e847d3 into dev Nov 15, 2024
18 checks passed
@blushi blushi deleted the feat-APP-323-project-filters-integration branch November 15, 2024 14:35
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.

4 participants