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

[FE/BE] - Mozfest Community Directory #7603

Merged
merged 19 commits into from
Dec 15, 2021

Conversation

b-ggs
Copy link
Collaborator

@b-ggs b-ggs commented Oct 11, 2021

Closes #7421 (Epic)
Related PRs/issues #7602 (BE pull request), #7422 (BE issue), #7417 (FE issue)

live link: https://mozfest-foundation-s-feature-fe-xuxj3g.mofostaging.net/en/

Link to sample test page:

Checklist

Remove unnecessary checks

Tests

  • Is the code I'm adding covered by tests?

Changes in Models:

Documentation:

  • Is my code documented?
  • Did I update the READMEs or wagtail documentation?

@mofodevops mofodevops temporarily deployed to foundation-s-feature-fe-ovhu8u October 11, 2021 16:12 Inactive
@b-ggs b-ggs changed the title Feature/fe be mozfest community directory [FE/BE] - Mozfest Community Directory Oct 11, 2021
@github-actions
Copy link

This PR introduces visual differences. Click here to inspect the diffs.

* Profile directory nav start

* Add isues to cards and filter for profile types

* Convert profile cards to tailwind

* directory listing styling

* Start react setup for tabbed profile directory filtering

* start adding subfilters to directory properties

* Create pulse profile component, hook up tabbed profile directory to api

* Filtering, back button, active button when no filter

* Dropdown styling for subfilters

* profile subfiltering functionality

* renaming

* Create components for profile directory react items

* Revert directory blocks to how they were

* Filters nav horizontal scroll on mobile

* Remove extra directory nav buttons used for testing

* Code cleanups as per feedback
@mofodevops mofodevops temporarily deployed to foundation-s-feature-fe-ovhu8u October 21, 2021 21:58 Inactive
@github-actions
Copy link

This PR introduces visual differences. Click here to inspect the diffs.

@mofodevops mofodevops temporarily deployed to foundation-s-feature-fe-ovhu8u October 21, 2021 22:19 Inactive
@github-actions
Copy link

This PR introduces visual differences. Click here to inspect the diffs.

Copy link

@sabrinang sabrinang left a comment

Choose a reason for hiding this comment

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

Pulse Filters
Filter value: it would be helpful that the exact values are defined in the Pulse api/admin in the help text too as I haven't used pulse filters in a long time. It would be great if options can be sorted so I don’t have to delete everything to reorder.

Tabbed profile directory
If I just choose a pulse filter in the component for Tabs only, I get I get Server Error (500) when I preview/publish if a Subfilter isn’t selected. Is this missing a validation error if I’m not able to do this or is this a required field?

Tabs

  • It wasn’t very obvious how to create another tab. I figured it out when it was done via the Pulse filter itself. I didn’t understand the help text: “The profile information to create tabs on. The first option in the snippet will be used as the initial filter.” does it mean ... that the Tabs are created based on the selected pulse filter and the first option in the snippet will be the first tab?
  • Mobile: Make it go to the edge so it hints that there is more (looks cut off with the margin right now)

image

Content

  • I’m not familiar with how the MozFest team wants to organize the content this year but for different tabs could you have different subfilters? For example, Wrangler filter by spaces then Volunteers filter by years? It may be good to check with whoever is entering to review content as I don't have the content and am not too experienced with pulse filters either which made reviewing the filtering functionality experience tricky as I didn’t want to actually update the pulse user profiles if I wasn’t suppose too but it would be most useful to test with real content.

Design

  • Filter drop down box: it is too narrow for the average space names or if they are localized as well (make this wider and on mobile as well)

Screen Shot 2021-10-22 at 3 26 59 PM

  • Filter check box: make it #595cf3 blue instead of purple

Screen Shot 2021-10-22 at 3 27 35 PM

  • Profile card bottom label: if these are not links/clickable, they should be black
  • Profile card on narrow 7-columns: the full-width version is preferable as the narrow photo proportions are not ideal but I saw the mockups desire having two up. If this is what is desired by the stakeholders, I'd atleast fix the alignment of the social icons (to be consistently upper right) to clean up those elements if they aren't specific of what they wanted I recommend having one profile up at a time.

image

@stevedya
Copy link
Collaborator

Hey @sabrinang I have made the necessary changes at least for the design feedback you had. I've been away from the project for the last little bit but I can put some time into getting this one finished up next week. Some of the functionality changes eg. Having the filter values defined or choosing different sub filters for each filter might need a bit more discussion though.

Copy link

@sabrinang sabrinang left a comment

Choose a reason for hiding this comment

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

@stevedya thanks for getting back to this! I'm looking at https://mozfest-foundation-s-feature-fe-ovhu8u.mofostaging.net/en/ and it looks like design front-end changes haven't been updated so I'm wondering if I'm looking at the wrong deployment?

Also, it looks like the wrangler section disappears when I click elsewhere and return to it:
ie4vX50bQb

commit 3530887
Author: Steve <[email protected]>
Date:   Thu Dec 2 16:16:39 2021 -0700

    Update filters nav spacing on mobile, adjust help text on Tabbed Profile Directory

commit 7ddbe3e
Author: Steve <[email protected]>
Date:   Wed Nov 3 22:48:30 2021 -0600

    Pulse profile social icons alignment

commit 45e62c3
Author: Steve <[email protected]>
Date:   Wed Nov 3 22:46:03 2021 -0600

    Fixes as per feedback, custom checkboxes, subfilter dropdown width

commit 03ceb29
Author: Steve <[email protected]>
Date:   Thu Oct 21 15:56:08 2021 -0600

    Code cleanups as per feedback

commit 17dc691
Author: Steve <[email protected]>
Date:   Wed Oct 20 16:16:07 2021 -0600

    Remove extra directory nav buttons used for testing

commit 7a27559
Author: Steve <[email protected]>
Date:   Wed Oct 20 16:07:39 2021 -0600

    Filters nav horizontal scroll on mobile

commit 0383eee
Author: Steve <[email protected]>
Date:   Wed Oct 20 13:42:30 2021 -0600

    Revert directory blocks to how they were

commit 73fa75e
Author: Steve <[email protected]>
Date:   Wed Oct 20 13:34:51 2021 -0600

    Create components for profile directory react items

commit 7ca188c
Author: Steve <[email protected]>
Date:   Tue Oct 19 22:26:17 2021 -0600

    renaming

commit 89df5e1
Author: Steve <[email protected]>
Date:   Tue Oct 19 22:03:47 2021 -0600

    profile subfiltering functionality

commit c067772
Author: Steve <[email protected]>
Date:   Tue Oct 19 01:25:16 2021 -0600

    Dropdown styling for subfilters

commit 8d4ebc6
Author: Steve <[email protected]>
Date:   Mon Oct 18 22:38:46 2021 -0600

    Filtering, back button, active button when no filter

commit 8785af7
Author: Steve <[email protected]>
Date:   Mon Oct 18 16:50:56 2021 -0600

    Create pulse profile component, hook up tabbed profile directory to api

commit d1bfcc4
Author: Steve <[email protected]>
Date:   Mon Oct 18 10:08:29 2021 -0600

    start adding subfilters to directory properties

commit f5b94ca
Author: Steve <[email protected]>
Date:   Fri Oct 15 16:22:07 2021 -0600

    Start react setup for tabbed profile directory filtering

commit e8f8147
Author: Steve <[email protected]>
Date:   Thu Oct 7 18:42:21 2021 -0600

    directory listing styling

commit be1170f
Author: Steve <[email protected]>
Date:   Thu Oct 7 18:13:18 2021 -0600

    Convert profile cards to tailwind

commit 7c94a66
Author: Steve <[email protected]>
Date:   Thu Oct 7 16:22:11 2021 -0600

    Add isues to cards and filter for profile types

commit 724bdcc
Author: Steve <[email protected]>
Date:   Wed Oct 6 12:42:34 2021 -0600

    Profile directory nav start
@stevedya
Copy link
Collaborator

stevedya commented Dec 3, 2021

Hey @sabrinang,

Thanks for looking at this again. I finally had some more time today to look at the outstanding issues.
Looks like I accidentally did that design change work on my previous FE branch 😣 so i moved those design updates to this one and gave it a push. Let me know if the updates still aren't showing. As per your original note these are the things I updated now:

Pulse Filters

  • No updates here but we will need a list of the filter values available in pulse if we are to list the available ones in the snippet

Tabs:

  • Updated help text to be clearer (similar to what you described)
  • Made the filters touch the edges on mobile

Screen Shot 2021-12-02 at 4 00 51 PM

Content

  • Im not sure if the mozfest team will need separate sub filters for each main filter at this time, however this would be a BE change for @richbrennan or @b-ggs

Design

  • Enlarged subfilters width to allow longer filter names and reduce wrapping
  • Changed profile card bottom labels to black as requested
  • Kept it as a 2 cards per column on the version with the sidebar for now, I can check with marc on this one what he prefers.
  • Adjusted alignment of social icons to sit at the top of the card instead of centered to card title text

Screen Shot 2021-12-02 at 4 10 07 PM

** Reply to your previous comment **

  • I think the issue happens when there is nothing to filter by so usually we wouldn't wanna include empty filters when configuring. However I think i fixed it and added a fall back for this just incase. Let me know if its still broken in that way
    Screen Shot 2021-12-02 at 5 21 02 PM

Let me know if the new changes work or if I will have to get this to trigger a deployment again 😃
Thanks!

@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-fe-4enrmf December 3, 2021 20:58 Inactive
@danielfmiranda danielfmiranda temporarily deployed to foundation-s-feature-fe-yb1gjr December 3, 2021 21:08 Inactive
@richbrennan
Copy link
Contributor

I've merged in the main branch and fixed the BE merge conflicts, however there are still conflicts in the following FE files:

  • mozfest.scss

  • tailwind.config.js

Copy link
Collaborator

@danielfmiranda danielfmiranda left a comment

Choose a reason for hiding this comment

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

Hi Everyone, I believe in order to build the test app we need the node and python tests to pass.

I tried to pull this PR into my local machine to see if I can get a fix, however I was unsure how to set up the tabbed directory and thus unable to test.

However I added some suggested fixes that I think would get the tests to pass.

Thanks in advance and please let me know if you have any questions about my comments!

@mofodevops mofodevops temporarily deployed to foundation-s-feature-fe-ljm9lm December 8, 2021 21:56 Inactive
@stevedya stevedya marked this pull request as ready for review December 8, 2021 21:58
@mofodevops mofodevops temporarily deployed to foundation-s-feature-fe-ljm9lm December 9, 2021 12:49 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-feature-fe-ljm9lm December 9, 2021 14:12 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-feature-fe-ljm9lm December 14, 2021 17:03 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-feature-fe-ljm9lm December 14, 2021 17:30 Inactive
@Pomax Pomax requested review from kristinashu and removed request for sabrinang December 15, 2021 17:40
@Pomax Pomax temporarily deployed to foundation-s-feature-fe-xuxj3g December 15, 2021 18:26 Inactive
@kristinashu
Copy link

Design-wise this looks good and it looks like Sabrina's feedback was addressed. Like Sabrina, I'm not familiar with the real content and setting up the different filters was confusing - I think it will be helpful for Marc to test it out further once it's landed.

Copy link

@kristinashu kristinashu left a comment

Choose a reason for hiding this comment

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

Design looks but but code review is still needed.

Copy link
Collaborator

@danielfmiranda danielfmiranda left a comment

Choose a reason for hiding this comment

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

Hi Everyone,

Thanks for tagging me in this. After taking a look, everything looks good to me and I am approving!

@Pomax Pomax temporarily deployed to foundation-s-feature-fe-xuxj3g December 15, 2021 22:50 Inactive
@Pomax Pomax dismissed sabrinang’s stale review December 15, 2021 23:00

taken over by Kristina

@Pomax Pomax merged commit b5e42ed into main Dec 15, 2021
@Pomax Pomax deleted the feature/fe-be-mozfest-community-directory branch December 15, 2021 23:00
@Pomax
Copy link
Contributor

Pomax commented Dec 20, 2021

Hey all - this PR duplicated code we already had in place for streamfield blocks, and should have had an issue filed for the fact that we now have duplicate code that is a source of future bugs when this code gets updated in only one place, leading to divergent behaviour between two parts that should behave the same. Hopefully we can remember to flag and file this in the future. Additionally, it looks like this new snippet cannot be regulated in terms of permissions, so even though the code landed, staff can't actually create/edit them without admin rights, so that's not great.

I've filed #8033 to address the code cleanup in the new year, and #8034 for the permission issue.

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.

Epic: Mozfest Community Directory
9 participants