Replace generated complex CSS with handcrafted CSS #291
Conversation
--------- Co-authored-by: Luis Fernando Pardo Sixtos <lpardosixtos@microsoft.com> Co-authored-by: lpardosixtosMs <94007365+lpardosixtosMs@users.noreply.github.com>
|
Comparison before and after the change. After: |
|
@rniwa @bgrins @flashdesignory PTAL. This looks like a big change, but it is mostly refactoring and adding things to the complex DOM, the most important files to review are We still need to remove the extra class names that we previously added to the standalone versions, but we can do it in a follow up PR. |
|
Styled popovers: |
|
From a high level this looks like a good change. In addition to being a simplification, having the rules apply visual effects (like prioritized items) is an improvement. I haven't gone through all the CSS in the PR but wanted to signal support from a first pass review. |
resources/todomvc/architecture-examples/angular-complex/scripts/build.js
Outdated
Show resolved
Hide resolved
resources/todomvc/architecture-examples/angular/src/app/todo-list/todo-list.component.html
Show resolved
Hide resolved
* Move reused logic into buildComplex Util function * Update build scripts
| fs.copyFileSync(sourcePath, targetPath); | ||
| } | ||
|
|
||
| if (cssFilePath) { | ||
| // get the name of the css file that's in the dist, we do this because the name of the css file may change | ||
| const cssFile = fs.readdirSync(path.join(callerDirectory, sourceDirectory, cssFolder), { withFileTypes: true }).find((dirent) => dirent.isFile() && cssFileNamePattern.test(dirent.name))?.name; |
There was a problem hiding this comment.
I can see that this project is ignored by eslint and prettier (see .eslintignore and .prettierignore), it would probably be good to not ignore the handcrafted files such as this one and the others such as the handcrafted CSS file.
Could you either add the handcrafted files or ignore just the generated files, and run the formatter?
There was a problem hiding this comment.
I'm good if you want to do this in a follow-up to more clearly distinguish the concerns.
There was a problem hiding this comment.
IIUC this project is not ignored, both the rules added to the .prettierignore and .eslintignore both start with "!".
I also see these files in the output when I run the formatter.
I can remove the rules I added previously in a follow-up since they aren't needed and I already have a follow-up PR in the works.
!/resources/todomvc/big-dom-generator
There was a problem hiding this comment.
oh right I got confused.
Our project's prettier is configured with printWidth: 250. I find this too long and got confused by this long line, but looks like this is accepted by prettier then...
Maybe you could split it up a bit still, up to you.
| cssFilePath, | ||
| cssFolder = "", // sometimes the css file we are looking for may be nested in another folder. | ||
| cssFileNamePattern, // The css file name pattern is used to find the css file in the source dist directory. |
There was a problem hiding this comment.
maybe write clearly in a comment that cssFileNamePattern is mandatory if cssFilePath is present? Maybe it could even be good to check this in the code and throw an intelligible error if that's absent, but I'll let you choose if you want to implement this improvement.
There was a problem hiding this comment.
Will include in the follow-up which touches these files, I like the improvement.
|
Thank you everyone for your reviews, merging this now. |
We are replacing the current auto-generated CSS rules for the complex DOM with a smaller number of handcrafted rules (see supporting doc.
Changes to the complex DOM generator:
big-dom-generator/src/app.css.big-dom-generator/utils/app.cssChanges to the standalone version:
data-priorityattribute to the todoMVC list items that will be used to provide additional styling using new handcrafted CSS.Changes to the complex versions:
Hosted version (with all complex todoMVC variations enabled): https://lpardosixtosms.github.io/SpeedometerComplexHandcraftedAll/?developerMode=&tags=todomvc&measurementMethod=timer#home