fix: ghost loaders height #292
Conversation
chintankavathia
requested review from
timowolf,
kfenner,
fh1ch and
spike-rabbit
as code owners
chintankavathia
force-pushed
the
fix/ghost-loaders
branch
2 times, most recently
from
c67e7b2 to
e5f80f8
Compare
| return rowHeight === 'auto' ? 100 / this.pageSize() + '%' : rowHeight + 'px'; | ||
| }; |
There was a problem hiding this comment.
After actually playing around with this, I think that in case of auto the calculation is not correct.
I guess ideally we would have 1em plus the padding of normal cells. But the padding is hard to get, as there is no variable for this. Can you please check if you can find a solution there? Otherwise we could also just assume stuff like 1.5em or so.
chintankavathia
force-pushed
the
fix/ghost-loaders
branch
from
e5f80f8 to
ec9f131
Compare
show border on each ghost element in case of cellMode is off and also hide outer ghost overlay if cellMode is on to avoid duplication.
chintankavathia
force-pushed
the
fix/ghost-loaders
branch
from
ec9f131 to
dee5ee1
Compare
There was a problem hiding this comment.
Definitely an improvement. Not sure how breaking this is in terms of styling.
What about just always using the cell / row classes in the ghost loader? Than we don't need the borders and stuff. The only problem here would be that custom styling breaks. So we may need to wait for a major release. @fh1ch WDYT?
@spike-rabbit I agree. I really like the changes. But if we go the proposed route and hence break custom styling (which seem to be not only used by us), this would indeed merit a breaking change. |
|
Then we should wait here until we start preparing v25 |
spike-rabbit
added
the
breaking-changes
ghost loader when rowHeight is set to
autoare broken.see https://siemens.github.io/ngx-datatable/#/paging-scrolling-novirtualization
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: