Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

WIP: Upgrade Semantic Ui to use Semantic-UI-Less #130

Merged
merged 21 commits into from
Oct 31, 2017
Merged

Conversation

andrewleyva
Copy link
Collaborator

problem statement

The build is heavy and slow and is integrated in such a way that updates could overwrite some of our code.

solution

  1. Use Semantic-UI-Less instead of Semantic-UI package to pull in CSS for Semantic to bundle with our additional components.
  • Remove Semantic-UI package and folder (removing Gulp entirely)
  • Add semantic-ui-less and less packages to npm
  • Update Builder Script to build less with less.render and ship to Lib folder
  1. Use Less-loader to replace the old watcher

Notes:

Currently we must use Less v2.7.2 because the latest (3.0.0-alpha.3) results in build errors for semantic-ui. And issue is open: Semantic-Org/Semantic-UI-LESS#30 (comment)

@@ -25,8 +26,7 @@ module.exports = {
get coverageDir () { return path.join(this.projectRoot, 'coverage') },
get distDir () { return path.join(this.projectRoot, 'lib') },
get projectRoot () { return path.resolve(__dirname, '..') },
get semanticDist () { return path.join(this.semanticPath, 'dist') },
get semanticPath () { return path.join(this.projectRoot, 'semantic') },
get semanticPath () { return path.join(this.projectRoot, 'semantic-ui-less') },
Copy link
Contributor

Choose a reason for hiding this comment

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

look into require.resolve

require.resolve('semantic-ui-less') ==> // /Users/andrew/octagon/node_modules/semantic-ui-less/index.less

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needed to use the full path

console.log(stdout)
console.log('Building Semantic Less')
const lessInputSource =path.join(this.semanticPath, 'semantic.less')
const lessInputStream = fs.readFileSync(lessInputSource).toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

await fs.readFile(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Andrew Leyva added 9 commits October 20, 2017 17:49
…src. Adjusted script to use symlinks to pull in semantic-ui-less from node_modules and our theme info form src
….md, updated build script to work with less-loader.

Styleguidist using Less-Loader doesn't like some files to be symlinked. Replaced most files in builder to be compied. Themes folder is symlinked so that changes you make in src are hotloaded. Theme.config was not able to be symlinked and if changes are made to it the project must be rebuilt.
@wa11-e
Copy link

wa11-e commented Oct 27, 2017

Woohoo! new surge deployment available for viewing! 🎉 surgereview29593329.surge.sh

@wa11-e
Copy link

wa11-e commented Oct 27, 2017

Woohoo! new surge deployment available for viewing! 🎉 surgereview29595186.surge.sh

@wa11-e
Copy link

wa11-e commented Oct 28, 2017

Woohoo! new surge deployment available for viewing! 🎉 surgereview29596187.surge.sh

@wa11-e
Copy link

wa11-e commented Oct 28, 2017

Woohoo! new surge deployment available for viewing! 🎉 surgereview29596237.surge.sh

@wa11-e
Copy link

wa11-e commented Oct 28, 2017

Woohoo! new surge deployment available for viewing! 🎉 surgereview29596284.surge.sh

@cdaringe
Copy link
Contributor

@andrewleyva (& @ggascoigne) updates!

  • i moved the tmp build dir to a true sys-wise tmp dir
  • given your CSS build is pretty fast and because i parallelized a lot of the steps, i was able to drop less-loader, and just do a full CSS rebuild on every change from src/semantic-ui-theme, vs watching the temp dir less file. if it becomes problematic, no worries, let's port it back. it honestly is probably a little slower, but, it's simpler. "changed theme? then new theme dumped into lib"

take a look at the static styleguide from @wa11-e (yours, or mine).

  • it looks like the icon on the sortable table isn't rendering.
  • if you run npm install && npm test in the latest branch, there are some actual diffs im observing too. not much! just some smallies. we need to loosen the image diff threshold in the test :$
    • button padding-left|right maybe have both increased? we should see what's up w/ that.

@wa11-e
Copy link

wa11-e commented Oct 31, 2017

Woohoo! new surge deployment available for viewing! 🎉 surgereview29667143.surge.sh

@wa11-e
Copy link

wa11-e commented Oct 31, 2017

Woohoo! new surge deployment available for viewing! 🎉 surgereview29675775.surge.sh

@cdaringe cdaringe merged commit 8ad42db into master Oct 31, 2017
@cdaringe cdaringe deleted the feat/suir-cleanup branch October 31, 2017 23:41
@andrewleyva
Copy link
Collaborator Author

Addresses issue #119.

@ggascoigne
Copy link
Member

I'm curious what the anticipated impact on TOC and Connect v4 will be with this change.

@andrewleyva
Copy link
Collaborator Author

@cdaringe and I ran Backstop and investigated any differences. This really just cleans up the build on the Octagon side. I do have plans to address the Bundle size, issue #120, which should have more of an impact.

@ggascoigne
Copy link
Member

Cool, I'll pull it into TOC when I return and run backstop there :) That'll be the real test 😁

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants