Modifier Support for more border related css#712
Modifier Support for more border related css#712sandeepjak2007 wants to merge 15 commits intovarabyte:devfrom
Conversation
Note we also disable KSP2 as we're waiting on them first to fix a bug that depends on a bugfix in the compiler (which won't be fixed until 2.2.0)
These do not return normal bodies and should not have serialization options.
…border-block-end-style`, `border-block-start-color`, `border-block-start-style`, `border-block-style`, `border-inline-color`, `border-inline-style`, `border-inline-end-color`, `border-inline-end-style`, `border-inline-start-color`, `border-inline-start-style`
|
@bitspittle @DennisTsar I’ve made significant changes to the |
# Conflicts: # frontend/compose-html-ext/src/jsTest/kotlin/com/varabyte/kobweb/compose/css/CssStylePropertyTests.kt
|
Just following up on this — I had reorganized the methods in the Border file alphabetically for better structure and readability. Whenever you get a chance, I’d really appreciate it if you could take another look and let me know if everything looks good. Please let me know if any further changes are needed. Thanks again for your time and support! |
|
Totally fair to ping me! I'll be taking this review on tomorrow and will follow up if there are any concerns. Sorry for the wait. |
|
(Haven't forgotten this, but for some reason when I pull it down locally to review it, it totally fails to rebase cleanly, even though the GitHub view looks fine. I need to cook dinner now so I'll revisit this later tonight) |
|
Sorry Sandeep, I'm not at all sure what's going on. The diff from this GitHub UI view looks like it should rebase cleany, but when I check things out locally to try your changes out inside my IDE, I just get merge conflict after merge conflict. I'll look at it more tomorrow; worst case I'll manually recreate the PR. No changes required this time, but I bet this issue is caused by how you're merging changes locally on your end while you're working. Merging causes your history to be filled with changes that Dennis and I made as if it was part of your own code. The GitHub UI seems to filter it out, but locally git has no idea where the code came from and I'm having a heck of a time figuring out which lines to merge. I think in general you should avoid merging or rebasing during PRs as much as possible. Once a PR is up, just respond to comments to it, adding new commits and that's it. (From the official docs). Occasionally, someone will check code in that you might need, so in that case, you have to update code. It can't be helped. In that case, locally, I will pull down latest dev branch, I will rebase my PR branch against it, I will add the requested changes, and then I will Are you familiar with the differences between rebasing and merging? |
|
OK finally got everything working and had a chance to look at your changes. So a lot of these border classes don't need to be created. Please review: https://github.com/varabyte/kobweb/blob/dev/frontend/compose-html-ext/src/jsMain/kotlin/com/varabyte/kobweb/compose/css/CONTRIBUTING.md#every-non-longhand-css-style-property-should-have-a-corresponding-sealed-interface The decision we came to is that only shorthand properties (the ones in the far-left column of the CSS property spreadsheet) should have a class created for them. The reason we came to that decision, which we actually noted in the CONTRIBUTING section, was because of border, actually. Anyway, what this means is that these are the only border CSS properties we plan on creating classes for, at least pre-1.0.
So, please remove classes like |
|
Hey Sandeep, just checking in. Everything OK? |
|
@bitspittle Thanks for checking in! Everything’s going well. I've been actively job searching, so I haven’t had a chance to dive back into it yet. From the comments, I realized I may have missed some styling standards while creating the separate CSS for sub-styles. I’ll recheck everything and aim to push the fixes by this weekend. Appreciate your understanding! |
|
@sandeepjak2007 no worries, glad you're doing well. Keep prioritizing the job hunt as necessary! |
bitspittle
force-pushed
the
dev
branch
2 times, most recently
from
5c34e2e to
e726ac5
Compare
bitspittle
force-pushed
the
dev
branch
4 times, most recently
from
9100fe4 to
174e259
Compare
2 participants
Added modifiers for
border-block-color,border-block-end-color,border-block-end-style,border-block-start-color,border-block-start-style,border-block-style,border-inline-color,border-inline-style,border-inline-end-color,border-inline-end-style,border-inline-start-color,border-inline-start-stylePlease take a look and let me know if any changes are needed. Thanks!