Skip to content

Dropdown fix#3662

Closed
DmitriiTsy wants to merge 4 commits intohackforla:gh-pagesfrom
DmitriiTsy:dropdown-fix
Closed

Dropdown fix#3662
DmitriiTsy wants to merge 4 commits intohackforla:gh-pagesfrom
DmitriiTsy:dropdown-fix

Conversation

@DmitriiTsy
Copy link
Member

@DmitriiTsy DmitriiTsy commented Oct 26, 2022

Fixes #3256

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

  • Created dropdown_filters.SCSS in this _path: /hackforla/website/sass/elements
  • Copied all styles from _project-filter.scss _(Path: hackforla/website/sass/components) to dropdown_filters.SCSS

@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 DmitriiTsy-dropdown-fix gh-pages
git pull https://github.com/DmitriiTsy/website.git dropdown-fix

@github-actions github-actions bot added Feature: Refactor CSS Page is working fine - CSS needs changes to become consistent with other pages role: front end Tasks for front end developers Complexity: Small Take this type of issues after the successful merge of your second good first issue labels Oct 26, 2022
@jdingeman jdingeman requested review from giroz and jyaymie October 26, 2022 02:25
@jyaymie
Copy link
Member

jyaymie commented Oct 26, 2022

Review ETA: 10/25/22 @ 11pm
Availability: 9pm - 11pm

@giroz
Copy link
Member

giroz commented Oct 26, 2022

ETA: 10/26/22
Availability: 2 hours

Copy link
Member

@jyaymie jyaymie left a comment

Choose a reason for hiding this comment

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

Great work, @DmitriiTsy! The functionality of the dropdown filters menus seems to be preserved after your changes were made. I did want to mention a few things:

  • On my code editor, I see the new file name with the file extension in lowercase (i.e., dropdown_filters.scss instead of dropdown_filters.SCSS). I don't know if that's what you're seeing on your end. @kathrynsilvaconway, does casing matter in this case?
  • I was wondering if the last action item in the issue was taken care of. I wasn't sure as I didn't see anything related to it in the comments.
  • In the future, I would include the issue number in your branch name so we can identify the issue number at a glance.
  • In the future, if there are no visual changes to the website, I would make a brief note that that's the case in the body of the PR.

I'm happy to approve once there's confirmation about that last action item!

Copy link
Member

@giroz giroz left a comment

Choose a reason for hiding this comment

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

Good job, Dmitrii. I see you copied the file over to file to the new location but you didn't change the name of file to the dropdown_filters.scss. At least this is what I saw in my vs code editor.

@MattPereira MattPereira self-requested a review October 28, 2022 19:43
@MattPereira
Copy link
Contributor

ETA: End of Day 10/28/22
Availability: 2 hours

MattPereira
MattPereira previously approved these changes Oct 28, 2022
Copy link
Contributor

@MattPereira MattPereira left a comment

Choose a reason for hiding this comment

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

Good work @DmitriiTsy ! 👍

It looks like you renamed _project-filter.scss and moved its location rather than creating a new file and deleting the old one as instructed in issue #3256. I agree with your approach as it removes the possibility of incorrectly copying the contents of _project-filter.scss into dropdown_filters.scss.

@DmitriiTsy
Copy link
Member Author

@giroz
I just double-checked it in VS code and commit history (file changed section)
Screen Shot 2022-10-28 at 13 15 30
Look's like everything is okay.

giroz
giroz previously approved these changes Oct 29, 2022
Copy link
Member

@giroz giroz left a comment

Choose a reason for hiding this comment

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

I see the change now. Stupid me.

@esantiano esantiano self-requested a review October 29, 2022 22:17
@esantiano
Copy link
Member

ETA: 10/29/22
Availability: 3-5PM

esantiano
esantiano previously approved these changes Oct 29, 2022
Copy link
Member

@esantiano esantiano left a comment

Choose a reason for hiding this comment

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

Hey @DmitriiTsy

Found the changes you made to the original file name - great and simple approach to this issue, it also passes the ABC's good job!

@arpitapandya arpitapandya self-requested a review October 30, 2022 22:43
@arpitapandya
Copy link
Member

ETA: EOD 2022-10-30
Availability: 2 hrs

Copy link
Member

@arpitapandya arpitapandya left a comment

Choose a reason for hiding this comment

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

Hey @DmitriiTsy Thank you for working on this.

  • Could you provide screenshot by running it locally? It is a requirement to test the site locally, and since this PR is related to CSS, I would look for if something isn't breaking the site.
  • Could you provide demo to the merge team? It can be me for this as I am reviewing.

Let me know if you need help.

@jdingeman jdingeman self-requested a review November 5, 2022 00:38
Copy link
Member

@jdingeman jdingeman left a comment

Choose a reason for hiding this comment

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

Hi all,
When testing in your local environment, it is a good idea to docker-compose down and then docker-compose up after your changes are finished. Docker has a tendency to run an old image if it runs into an error if it's already running, and won't shut down.
For this issue, running on my local environment, running docker-compose up after switching into the branch caused it to exit because it cannot find the _project-filter.scss file, causing main.scss to import a nonexistent file.
image

In order for this to not break the page, you will need to import the newly created dropdown_filters scss file into main.scss.

Also, please note that, for naming conventions, the name of the filer should actually be _dropdown_filters.scss with the underscore in front.

@DmitriiTsy
Copy link
Member Author

Thank you so much @jdingeman. Now it works fine.

@DmitriiTsy
Copy link
Member Author

ETA: 2022-11-12
Availability: 2 hrs

@DmitriiTsy DmitriiTsy dismissed stale reviews from giroz and MattPereira via fa27463 November 12, 2022 18:17
@DmitriiTsy
Copy link
Member Author

@arpitapandya

Hi, sure.

Here is Docker container
Screen Shot 2022-11-12 at 10 20 07

Here is the website:
Screen Shot 2022-11-12 at 10 20 42

Everything works fine for now. I can do a real-time demo with Zoom or send you a record - please let me know which is better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complexity: Small Take this type of issues after the successful merge of your second good first issue Feature: Refactor CSS Page is working fine - CSS needs changes to become consistent with other pages role: front end Tasks for front end developers status: Updated No blockers and update is ready for review

Projects

Development

Successfully merging this pull request may close these issues.

Create filter_dropdown.scss File in Elements Folder

7 participants