code-for-chapel-hill / NC-COVID-Support

Community support site, supporting COVID-19 business opening hours, Food Banks, School Meals, Farms and Social Services.
https://nccovidsupport.org
GNU General Public License v3.0
16 stars 39 forks source link

Issue 246 (Issues with PurgeCSS) #257

Closed joonyoungleeduke closed 3 years ago

joonyoungleeduke commented 4 years ago

The normal styling lost with PurgeCSS was reimplemented individually as follows:

1) The caret appearing on the dropdown menus was removed by setting 'appearance: none' under the .custom-select class in the Main.scss file.

Screen Shot 2020-07-10 at 9 18 32 PM

2) The borders around the dropdown menus was removed by adding a 'border-fix' class to the relevant DOM element in SearchFilter.vue.

Screen Shot 2020-07-10 at 9 20 04 PM Screen Shot 2020-07-10 at 9 20 14 PM

3) The 'NC COVID Support' styling issue was fixed by adding the styling class ('navbar-brand') for the DOM element into the code in Header.vue.

Screen Shot 2020-07-10 at 9 21 16 PM

With these 3 changes, the website's styling is back to normal with the added benefit of PurgeCSS removing unnecessary files.

Git rebase was completed, and it is from a fork of the branch mentioned in Issue 246 (where PurgeCSS was added).

ghost commented 4 years ago

Hey there! :wave: This repository uses the Prettier code style.

You forgot to format these files in your pull request:

The Prettier installation guide is a good place to get started with formatting your code properly. Thanks!!

:heart:

Your friendly Prettifier bot

joonyoungleeduke commented 4 years ago

I ran 'npm run format' so I'm not entirely sure why prettifier is flagging this.

readingdancer commented 4 years ago

I ran 'npm run format' so I'm not entirely sure why prettifier is flagging this.

We are not sure either, there seems to be something not quite right about that plugin, it might be related to line endings if you are using a Mac? We have seen this happen a few times, so it's a bit annoying, maybe one day we will figure it out :)

readingdancer commented 4 years ago

Hi @joonyoungleeduke

Thank you for submitting the PR, unfortunately it doesn't seem to be fixing all the issues. Also since this ticket was raised our Master branch has actually moved on quite a bit, so I think we probably need to re-base from the Master branch and then compare to the current master to your code, to see what breaks.

Here is a screen shot of the live site and my local version of your PR:

image

I have tried to highlight all the issues for you.

You can either try and fix these issues, or, we could try and rebase this PR on top of the current Master branch and fix whatever issues are then found, either way, we need to merge this into the Master branch soon before they start getting too far apart :)

Cheers, Chris

readingdancer commented 4 years ago

@all-contributors add @joonyoungleeduke to code

allcontributors[bot] commented 4 years ago

@readingdancer

I've put up a pull request to add @joonyoungleeduke! :tada:

joonyoungleeduke commented 4 years ago

Hi @joonyoungleeduke

As I just mentioned in my comment, for this PR to be merged in, we really need to get it 100% working in the same way as the current live site.

Let me know which way you'd like to proceed and if you need any help?

Cheers, Chris

I'll try to go with my original strategy and fix the issues that you have pointed out. First, I'll try and see if they're the same on my end and then try to fix them as soon as possible.

joonyoungleeduke commented 4 years ago

I added more Purge CSS ignore comments on (what I believe) to be all the style files. Additionally, I added the appearance: none styling to the .custom-select class inside of the SearchFilter.vue, mimicking the styling applied to the same class inside Main.scss; hopefully, that fixes the caret appearance for non(?) dark mode.

The changes overall seem to have fixed glaring mistakes, as in the following screenshot, except for the navigation button, which I am not entirely sure where its size is set to be 26 x 26 px (current web release size) instead of 30 x 30 px.

Additional Note: my local run for some reason does not have the Poppins font, explaining the following screenshot's text oddity. The Poppins font may cause changes in the formatting that I have not been able to reproduce as my local run, as aforementioned, has problems loading the font.

Screen Shot 2020-07-11 at 7 15 06 PM
readingdancer commented 3 years ago

Hi @joonyoungleeduke

I am sorry that this PR was never merged into the solution, I really appreciate that you spend a good amount of time on it. The reason I didn't manage to merge it is was that there were so many other changes at the time it would have been a really messy merge. If you are interested in helping with this project again, we are looking to pick it up again and try to move it forward as we all dropped the ball for the last 6 months due to work / COVID and everyone's lives in general.

Best wishes,

Chris