SASS upgrade - and extra header row removed#4809
Conversation
* added "rollup-plugin-sass" to handle the new sass library's roll-up function. * updated eslint to 9.3.6 (because there are serious breaking changes in SASS so need a new version of eslint). * removed extra header row from FreezeRows.js (code commented - not sure if it should be deleted) Notes: - 1. function4.scss was merged with variables4.scss because they depended on each other (new SASS can't handle circular dependencies) 2. function5.scss was merged with variables5.scss (same reason) 3. linting options are now in eslint.config.js 4. css maps are no longer generated - don't know why.
…d of a css file * restored origional rolllup plugin for stylesheet processing
|
Hi people, Sorry - i see that my initial post is confusing. Let me try jump out of the details a bit - and give some context. and i wanted to fix it. The fix is easy - but i found that i could not build the project because npm would not let me install node-sass (since it's deprecated). So - then i tackled the whole "migrate to sass" issue. So now - the pull request is mostly about migrating from node-sass to sass (but the un-wanted The rest of the detail in the first post is about how I tried to make the changes as small as possible. |
|
@David-Mawer Wow, thanks for the effort! This is huge. I will give a proper review and testing tomorrow. For now, we definitely don't want the dist folder to be merged, so it'd be great if it's removed. :) |
|
Hi @azmy60 - have removed the dist branch changes. |
| this.topElement.classList.add("tabulator-frozen-rows-holder"); | ||
|
|
||
| fragment.appendChild(document.createElement("br")); | ||
| //fragment.appendChild(document.createElement("br")); |
There was a problem hiding this comment.
I don't think it's a good idea to comment this right now. It was added for preventing a rendering issue based on this commit.
I'm not sure what rendering issue it was honestly, so it'd be awesome if @olifolkerd could help us with some hints?
@David-Mawer, also would it be enough to hide <br> from your css instead? For example:
.tabulator-header .tabulator-header-contents > br {
display: none;
/* Or play with other props like `height` maybe */
/* height: 0; */
}There was a problem hiding this comment.
Hey @azmy60,
I could make the change that you suggested, but then any grids i have with Frozen rows will not render correctly.
My thinking was to try preserve the Frozen Rows rendering. This was the approach: -
So - from the code it looks like the "<br/>" was intended as part of the "Frozen Rows" class thing.
From the rest of the code in FrozenRows.js - it looks like that initial break is needed for some reason at the top of the section.
Since i don't have frozen Rows in my one grid - i moved the line break using the following mechanism: -
- Commented out line 30 that we are discussing.
- Changed the css for the "tabulator-frozen-rows-holder" class so that has a padding-top: 1em (this is line in "tabulator.scss" - line 428; and in "tabulator-materialize.scss" - line 88).
So - basically removed the line break (which shows all the time - even when there are no frozen rows),
and updated the frozen rows holder class to have it's own built-in line break through padding.
The idea is that FrozenRows still render correctly, and also no-FrozenRows renders without the annoying padding at the bottom of the header.
There was a problem hiding this comment.
Oh I didn't see the new padding-top. Okay, I think the sass upgrade looks good overall (Great job!). I'll be merging this soon hopefully.
And for the commented <br> line, could you add a note above it explaining why this was commented, just for future context? Thanks!
azmy60
left a comment
azmy60
left a comment
There was a problem hiding this comment.
There were still issues with the SCSS variables not being overridden correctly, so some styles were broken. I've added the fix so it looks good now. Thanks for the amazing PR!
| this.topElement.classList.add("tabulator-frozen-rows-holder"); | ||
|
|
||
| fragment.appendChild(document.createElement("br")); | ||
| //fragment.appendChild(document.createElement("br")); |
There was a problem hiding this comment.
Oh I didn't see the new padding-top. Okay, I think the sass upgrade looks good overall (Great job!). I'll be merging this soon hopefully.
And for the commented <br> line, could you add a note above it explaining why this was commented, just for future context? Thanks!
2 participants
Overview of what this PR does: -
this was my original aim because of "Unwanted
element rendering after column headers in tabulator-header Unwanted <br> element rendering after column headers in tabulator-header #4783"
Notes on the details: -
I could not find a plugin using the newer "modern" API to generate a css file (the closest was "rollup-plugin-sass" - but it generated javascript code wrapping the CSS styles (all within the css file).
Maybe this is the new "modern" API - I don't know.
Because those changes are included in this pull request - but formatted a differently.
Total of 28 files changed (the rest are in the dist folder).
Sorry that I committed the dist folder changes (they were generated by the app). If this is a problem - I'll remove those changes from the pull request.
Finally - i didn't mean to make such a big PR. Was just trying to make a small change in 2 files.
The big PR was because I couldn't install node-sass (it's been deprecated, and my npm would not allow the install).
Despite all the changes - I'm pretty sure that I've preserved the styling.
(also - all the tests are passing)