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

Feature/CSS refactoring #832

Open
wants to merge 98 commits into
base: main
Choose a base branch
from

Conversation

adamplesnik
Copy link

@adamplesnik adamplesnik commented Apr 19, 2023

Description

My original intention was to introduce a dark mode for webumenia.sk. However I found out that the project CSS was due to an update and I attempted to do one.

List of changes

  • add current Tailwind CSS to the project, update its build
  • remove all old and unused LESS and static CSS files
  • replace legacy Tailwind and Tachyons CSS with Tailwind only classes
  • replace some Bootstrap classes with Tailwind
  • optimize style and admin CSS builds

Todo

  • use npm versions of plugins (eg. Bootstrap, CKeditor, etc.), remove its code from the repository
  • introduce CSS properties
  • dark mode

Type of change

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have updated the CHANGELOG
  • I have requested at least 1 reviewer for this PR
  • I have made corresponding changes to the documentation

Adam Plesnik added 7 commits April 18, 2023 09:13
Required when working with Webstorm or other Intellij IDEs.
Update both files, add newer version of Tailwind CSS and fix some npm vulnerabilities.
Remove .tailwind-rules container. We don't need it because all Tailwind clases use the 'tw-' prefix.

Remove @layer base {} from CSS.
We include and build Tailwind CSS properly and there is no need for this file anymore.

Rename all tailwind-like-utils classes to their 'tw-' equivalents.
Remove these files and replace its classes with Tailwind CSS variants.
* Delete some obsolete and no longer used static CSS
* Move admin CSS files to public/css/admin
* keep only build files in the root folder
* delete obsolete and unused files
* tune file structure
@adamplesnik
Copy link
Author

Feedback is welcome.

@igor-kamil
Copy link
Member

Wow 🤩 . This PR looks like a miracle. We were thinking about doing all of these things before... but nobody really wanted to start. I appreciate your effort a lot.
We need some time to walk through all your changes (which are huge!). And double-check any possible issues.
We will definitely give you feedback soon.
Thanks a lot for this. You made my day.... ✨

Adam Plesnik and others added 5 commits April 20, 2023 10:06
* remove obsolete files
* keep admin CSS in public/css/admin.
* keep site CSS in public/plugins
* move all CSS resources to resources/
* keep only generated CSS in public/css/
* put all imports into admin.less and style.less respectively, remove it from PHP
* remove more unused less files
* include compiled Tailwind in main style files
* remove app-tailwind.css
It is messing with Tailwinds' rem units.
@adamplesnik
Copy link
Author

Thank you for the feedback. I am glad you like the direction of my efforts.

This phase is IMO done, I would like to eventually meet with you someday and talk about testing this PR and future development.

I read something about dropping / replacing the Bootstrap (in the commit messages): this is the task I would love to help you with. I also like to 'atomize' (Tailwind-ize 😄) the CSS and to work on overall refactoring.

Btw I love all your work in lab.sng. I checked the Warhol microsite in the meantime and it blew my dekel off; splendid work!

@eronisko
Copy link
Contributor

Hi @adamplesnik 👋!

Thank you very much for your effort. It'll take a while for us to review all the changes, but from skimming the code, it looks like a major step in the right direction.

Right now I'm having trouble compiling the project assets (see e.g. here). Are you able to run npm run dev locally?

Note the project uses NodeJS v14 right now. That's is not a hard requirement AFAIK but if you're using a different version, please let us know.

Adam Plesnik added 2 commits April 21, 2023 14:38
…S to be built with postCss.

+ fix import paths
# Conflicts:
#	package-lock.json
#	package.json
@adamplesnik
Copy link
Author

Hi @eronisko, thank you for the feedback.

I actually broke the build by calling Tailwind plugin in less() build. I put Tailwind into separate file, it should work now.

Adam Plesnik added 3 commits April 21, 2023 14:51
…arate CSS to be built with postCss."

This reverts commit 2a1ac52.
* create separate CSS to be built with postCss.
* fix import paths
* no code reformatting this time ;)
@adamplesnik
Copy link
Author

I am sorry about the revert, I realised too late that it would be better to leave the code formatting off for PHP files.

Adam Plesnik added 5 commits April 21, 2023 15:00
I am sorry I was in a hurry and forgot to do things properly.
* create separate CSS to be built with postCss.
* fix import paths
* no code reformatting this time ;)
@adamplesnik
Copy link
Author

I am sorry about this commit mess, I reverted the initial build and then reapplied it in a hurry.

adamplesnik and others added 2 commits October 8, 2023 21:29
@adamplesnik
Copy link
Author

@eronisko I tried to fix all major issues. Would you please run the tests again? Thank you very much.

# Conflicts:
#	resources/js/components/user-collections/SharedCollection.vue
#	resources/views/components/footer.blade.php
#	resources/views/dielo.blade.php
#	resources/views/frontend/catalog/index.blade.php
#	resources/views/frontend/shared-user-collections/show.blade.php
#	resources/views/informacie.blade.php
#	resources/views/livewire/newsletter-signup-form.blade.php
#	resources/views/reprodukcie.blade.php
@eronisko
Copy link
Contributor

eronisko commented Oct 9, 2023

Thanks @adamplesnik. I'll get you the test results first thing on Thursday.

@eronisko
Copy link
Contributor

@eronisko eronisko changed the base branch from develop to main December 21, 2023 09:59
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.

3 participants