Create filter dropdown scss file in elements folder#3887
Conversation
|
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. |
|
Hi @wendybarrios thank you for taking this issue! I just want to know if this is fixing issue #3256 rather than #3662. ETA: 1 hour |
d-perez-8
left a comment
There was a problem hiding this comment.
Hello @wendybarrios, thank you for taking on this issue! I have noticed a couple of things when i was reviewing:
- There was an extra line space added to
assets/css/main.scss. Please delete the extra line since it is outside the scope of the issue. - I wasn't able to access the website with the changes, but I think is from an extra space at the end of the
_sass/elements/_dropdown-filters.scssfile. I think this is why there is a linter issue when you submitted the pull request.
Other than those 2 things, the pull request looks good!
|
Hello @d-perez-8, I made the requested changes.
|
|
How interesting. I am going to get the merge team to take a look at this linter error. @hackforla/website-merge Otherwise, the website is running fine. |
There was a problem hiding this comment.
Appreciate all your hard work on this one @wendybarrios
I noticed that your PR changes the filename to _project-filter.scss , but the issue instructs the filename to be _dropdown_filters.scss. Given that your PR creation comment refers to _dropdown_filters.scss , ehlp me understand why you decided to use the - instead of _ ?
Also, I am not seeing any linting errors on my machine @d-perez-8 , could you add screenshots to this issue?
This comment was marked as resolved.
This comment was marked as resolved.
|
@d-perez-8 my bad I totally missed that failing "Lint SCSS" check I'm not sure if the issue is actually in the I think I recall @jdingeman battling the PR Lint Checker in the past, but I have no idea how to fix this |
I have not yet found out why it fails the Linter. It seems like any PR that affects the .scss files tend to fail the linter due to style errors? It'd be a good idea to investigate the linter further. |
|
@MattPereira Regarding the name _project-filter.scss, it's a typo on my end. I was playing around with files names when I was having an issue with Docker but I could change - to _ to see if that may help with the Linter issue. But the other SCSS files under the elements folder use - rather than _ so I'm not sure sure what to use. What do you suggest? |
|
In situations like this, I lean towards using the issue action items as the source of truth. That way if it ends up being wrong you can blame the issue creator 🤣 |
|
@MattPereira Sounds good : ) I'll make the changes to match the issue action items. |
MattPereira
left a comment
There was a problem hiding this comment.
Fantastic work on this PR @wendybarrios ! 👍
Really appreciate all of your communication with the team and timely handling of all requested changes
There was a problem hiding this comment.
Hey @wendybarrios
We think the Lint SCSS Error might be caused by any changes made to the assets/css/main.scss file including the spaces you added.
Please revert all changes you made to assets/css/main.scss so that your PR only shows 2 files changed _sass/main.scss and _sass/components/_dropdown_filters.scss
If that doesn't work we will merge this PR and create another issue to investigate further.
|
@MattPereira I reverted all changes in assets/css/main.scss to its original state. I'm not sure if I did it correctly but I'll appreciate it if you can double check if it's right. Hopefully, it fixes the lint error. |
MattPereira
left a comment
There was a problem hiding this comment.
The linter error is fixed! 🥳
Reverting a file you already committed is not easy. Well done @wendybarrios 👍
|
@MattPereira Yay!, thanks for the help on this issue Matt! |
Fixes #3256
What changes did you make and why did you make them ?