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

WIP: convert container and component editor css to less - remove suit… #635

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 15, 2017

Attempts to convert root/index.css in editor to less (and resolving all linting issues). Breaks and looks like the following with a clean build:

brokencss

None of the css is generating. I cannot work out why this is happening. Would someone mind looking at my webpack and css set up to see what is going on? Thanks.

I have fixed the suitcss issues so it is not that causing the problems. No console feedback either, but the css/les files are definitely not being loaded. Please note, I could get my changes to work until I restart my machine and run a clean build

JIRA issue URL: paste URL here

[description]

Checklist

  • JIRA link
  • Check target branch
  • Make sure all commit statuses are green or otherwise documented reasons to ignore
  • QA needs to evaluate against the JIRA ticket
  • Changed files and commits make sense for this PR

See Zanata Development Guidelines more for information.


This template can be updated in .github/PULL_REQUEST_TEMPLATE.md


This change is Reviewable

@ghost ghost self-assigned this Dec 15, 2017
@codecov
Copy link

codecov bot commented Dec 15, 2017

Codecov Report

Merging #635 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #635   +/-   ##
=========================================
  Coverage     33.05%   33.05%           
  Complexity     5743     5743           
=========================================
  Files          1576     1576           
  Lines         62186    62186           
  Branches       7325     7325           
=========================================
  Hits          20554    20554           
  Misses        39715    39715           
  Partials       1917     1917
Impacted Files Coverage Δ Complexity Δ
...r/zanata-frontend/src/frontend/app/editor/index.js 0% <0%> (ø) 0 <0> (ø) ⬇️
server/zanata-frontend/src/frontend/app/index.js 0% <0%> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02f7c6a...77566f9. Read the comment docs.

@ghost
Copy link
Author

ghost commented Dec 18, 2017

Unfortunately the editor css must stay as css and not .less files because LESS + relative URLS + css-loader is incompatible (see here: webpack-contrib/less-loader#109 (comment))

Must be why editor was done purely in css to begin with.

Can slowly migrate the editor code to the frontend styles.less by using a wrappers like this:

#editor { //css here }

but I think they should be separate to reduce page load times. The less files I tried to create could not use the variables.less file so the inline variables in the editor css files will have to suffice for the moment. Any less css from editor will not generate - they are replaced by style.less from frontend.

I think as a practice the editor css and frontend css should be seperate until such times that we are using the same components across frontend and editor.

TLDR: Keep frontend and editor css directories. Isolate css to component level where possible - about optimisation above everything else. Any attempt to turn editor css to less results in missing css.

If any engineers can resolve webpack so it can handle the editor less (from container/Root/index.less) in this branch it would be greatly appreciated.

@ghost ghost closed this Dec 18, 2017
@ghost ghost added On hold and removed Work in progress labels Dec 18, 2017
@ghost ghost deleted the editor-css2less branch February 1, 2018 22:19
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0 participants