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

Reverts red filter tags on "Projects" and "Home Page" to prior location #4760

Merged
merged 6 commits into from
Jun 1, 2023
Merged

Reverts red filter tags on "Projects" and "Home Page" to prior location #4760

merged 6 commits into from
Jun 1, 2023

Conversation

agosmou
Copy link
Member

@agosmou agosmou commented May 31, 2023

Fixes #4753

What changes did you make and why did you make them ?

  • moved the filters tab to the sidebar to allow for a future search bar

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied

/# and /projects

image

/projects-check

image

Visuals after changes are applied /# and /projects

image

/projects-check

image

@github-actions
Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b agosmou-revert-filters gh-pages
git pull https://github.com/agosmou/website.git revert-filters

@github-actions github-actions bot added Bug Something isn't working role: front end Tasks for front end developers Complexity: Large time sensitive Needs to be worked on by a particular timeframe P-Feature: Home page https://www.hackforla.org/ P-Feature: Projects page https://www.hackforla.org/projects/ size: 0.5pt Can be done in 3 hours or less labels May 31, 2023
@agosmou agosmou requested a review from t-will-gillis May 31, 2023 05:36
Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

Hey @agosmou Thanks so much for working on this on short notice!

Some questions/ observations:

  • The Clear All button displays inline with the red filter tags rather than stacking under the tags like it was originally. I believe this is due to the edits in _dropdown_filters.scss around line 186. I feel that keeping the flex-direction:column and stacking the button under the filter tags looks cleaner. Was there a reason for changing this to flex-wrap:wrap?
  • The current filter column width, line 46 in the scss file, is set to min-width: 282px which I believe looks good for a tablet. I think I can see why you changed this to width: ---px but instead of 390px I believe that 290-300px would look better (plus the 'Amazon Cognito...' line won't wrap > 285px). Your thoughts?
  • Only an observation: In current-projects.js, for the insertAdjacentHTML I see you changed this from 'afterbegin' to 'beforebegin'; however I don't see that this changes the appearance/ placement of the 'Applied Filters' text.

Many thanks again for working on this!

CURRENT:
Screenshot 2023-05-31 110619
from PR 4760:
Screenshot 2023-05-31 110737
Column 290px, flex-direction: column:
Screenshot 2023-05-31 113302

@roslynwythe roslynwythe self-requested a review June 1, 2023 02:30
@roslynwythe
Copy link
Member

Availability: 5/31 10 - 11:30, 6/1 10 - noon
ETA 6/1 EOD

@agosmou
Copy link
Member Author

agosmou commented Jun 1, 2023

Hey @t-will-gillis

I've made all the changes per your suggestions. Thanks for checking this through! Take a look at the below and lmk if anything else comes to mind. I have an open day tomorrow so I can address items immediately.

  • filterDiv width reverted to min-width:282px
  • reverted to flex-direction: column
  • reverted the js now that column direction is re-implemented

The Clear All button displays inline with the red filter tags rather than stacking under the tags like it was originally. I believe this is due to the edits in _dropdown_filters.scss around line 186. I feel that keeping the flex-direction:column and stacking the button under the filter tags looks cleaner. Was there a reason for changing this to flex-wrap:wrap?

I originally changed it to flex-wrap to account for column stacking on the /projects-check page, but it looks like this page is inactive (should we remove it?)

image

The current filter column width, line 46 in the scss file, is set to min-width: 282px which I believe looks good for a tablet. I think I can see why you changed this to width: ---px but instead of 390px I believe that 290-300px would look better (plus the 'Amazon Cognito...' line won't wrap > 285px). Your thoughts?

I had widened it to account for the full width of the applied filter. Although the sidebar labels are intact, the applied filter for "Amazon Cognito" is slightly cut off.

image

Only an observation: In current-projects.js, for the insertAdjacentHTML I see you changed this from 'afterbegin' to 'beforebegin'; however I don't see that this changes the appearance/ placement of the 'Applied Filters' text.

Because of the flex-wrap style, the smaller filter buttons would end up adjacent to the Applied filters text, so I pulled it out of the "affected" HTML. I just came to see this remedy was not adequate because this text showed in the hamburger menu on mobile navigation.
The original column direction is reinstated so this is no longer an issue.

@agosmou agosmou requested a review from t-will-gillis June 1, 2023 04:43
Copy link
Member

@t-will-gillis t-will-gillis left a comment

Choose a reason for hiding this comment

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

Hey @agosmou - This looks great to me. The only question I have is with the min-width in the _dropdown_filters.scss. I see why you were using the width of 390px- anything less than this the 'Amazon Cognito...' tag is not completely visible. And anything less than 310px and the 'Diversity/ Equity/Inclusion' is not completely visible.

I will mark this 'Approved' but: @roslynwythe when you are looking at this, we can talk again about the minimum width if needed.

Otherwise thanks again for knocking this out so quickly!

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

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

Hi @agosmou thank you for your prompt attention to this issue. I noticed that if sufficient filters are applied so that no project cards are displayed, div.filtersDiv shifts to the center. Please advise @t-will-gillis should this be addressed within this issue?

image

@agosmou
Copy link
Member Author

agosmou commented Jun 1, 2023

@roslynwythe

Let me take a look this afternoon - my gut instinct is:

If no cards:
Insert a div with text "0 items found"

@roslynwythe
Copy link
Member

@roslynwythe

Let me take a look this afternoon - my gut instinct is:

If no cards: Insert a div with text "0 items found"

Hi @agosmou that approach sounds good to me

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

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

Hi @agosmou @t-will-gillis - I discussed the PR with Will and we decided to approve the PR as it is, and to use the ER process to address the issue that occurs when no cards are displayed. We will ask Bonnie about the ideal filter width on Sunday, and she may direct us to write another ER to address that.

I encourage you to write the ER(s) and also consider writing the associated issues, especially if you are interested in progressing onto the merge/dev lead team. Will or I can assist you.

In any case, thank you for great (and quick) work on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Complexity: Large P-Feature: Home page https://www.hackforla.org/ P-Feature: Projects page https://www.hackforla.org/projects/ role: front end Tasks for front end developers size: 0.5pt Can be done in 3 hours or less time sensitive Needs to be worked on by a particular timeframe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revert red filter tags on "Projects" and "Home Page" to prior location
3 participants