Conversation
| "dependencies": { | ||
| "core-js": "^3.23.2", | ||
| "vue": "^3.1.0" | ||
| "bootstrap": "^5.3.5", |
There was a problem hiding this comment.
Why do you add bootstrap as an npm dependency when we only link Bootstrap via CDN? See below:
kaitai_ci_ui/public/index.html
Line 10 in ebabd33
kaitai_ci_ui/public/index.html
Line 18 in ebabd33
Note that until now, we didn't have bootstrap as a dependency in package.json and yet everything worked fine, because we were also using Bootstrap only via CDN (so that's proof that it's not needed).
Installing bootstrap as an npm package is only needed if you want to "compile it from source", usually in order to be able to customize default Bootstrap settings, see for example https://getbootstrap.com/docs/5.3/customize/options/.
| }, | ||
| "extends": [ | ||
| "plugin:vue/essential", | ||
| "plugin:vue/vue3-essential", |
There was a problem hiding this comment.
Can you explain why was this needed? I'm looking at https://eslint.vuejs.org/user-guide/#bundle-configurations-eslintrc and I only see this:
Configurations for using Vue.js 3.x:
"plugin:vue/essential"... base, plus rules to prevent errors or unintended behavior.- (...)
Configurations for using Vue.js 2.x:
"plugin:vue/vue2-essential"... base, plus rules to prevent errors or unintended behavior.- (...)
It probably works the same, I'm just confused that there's no mention of plugin:vue/vue3-essential in the official docs. It's possible that it used to be called plugin:vue/vue3-essential initially before Vue 3 was universally adopted (when it co-existed with Vue 2), but now that Vue 3 is the only supported version of Vue, they renamed it to just plugin:vue/essential.
| <td v-bind:class="cssClassObject" v-bind:style="mixedBgGradientStyle" @click="details = !details"> | ||
| {{ data.status || 'unknown' }} | ||
| <div class="add-info" v-if="details && hasDetails" v-on:click.stop> | ||
| <div class="add-info position-absolute bg-white border" v-if="details && hasDetails" v-on:click.stop> |
There was a problem hiding this comment.
The add-info class already sets position: absolute, so why was the position-absolute class added here?
kaitai_ci_ui/src/components/CiCell.vue
Lines 152 to 153 in ebabd33
| height: 2px; | ||
| background: #ddd; | ||
| .sticky-top th { | ||
| border-bottom: 2px solid #ddd; |
There was a problem hiding this comment.
There was actually a reason why I used the :after pseudo-element to simulate a bottom border, instead of actually using border-bottom (I see that I should've left a comment explaining it). The border-bottom only works when the header row is at the original position at the top of the page, i.e. when the you haven't scrolled yet. As soon as you scroll down, the header row gets in the sticky position and the border-bottom is no longer visible. This is not an issue with the :after pseudo-element used before, see screen captures:
Before this PR
After this PR
In principle, I followed this answer from Stack Overflow that suggested using :after. As you can see, there are other answers, maybe some of them simpler than what I came up with, but I'd say that there's nothing wrong with :after - it gets the job done. We have more important things to do in Kaitai Struct.
| <style scoped> | ||
| .has-details { | ||
| cursor: pointer; | ||
| position: relative; |
There was a problem hiding this comment.
Why? The has-details was really just supposed to set cursor: pointer to communicate to the user that this cell in the CI matrix can be clicked on because it has details. I don't see any reason for position: relative.
There was a problem hiding this comment.
To be honest, I like the old appearance of the details box much better (on the left is before this PR, right is after):
After your changes, the details box has no border anymore (why?) and the message/stack trace listings all have white background with no border (why?). Also the padding of the target label and status (for example "zulu7-linux-x86_64: failed") is absurdly large in my opinion (visually, it looks like its height is double of what it was before).
|
@GreyCat BTW, why did you decide to upgrade Bootstrap? I kind of struggle with finding something positive about this PR (other than using a higher version number, which... has nothing to do with whether it's better or worse), I just see a bunch of small (visual) regressions that bother me a bit. I tried to point out some of them above - there are a few more that I don't care about that much, but to be honest I still think the old version looked better overall. |
2 participants
No description provided.