glen/dyna3
1
0
Fork 0
Code Issues 11 Pull Requests 1 Actions Projects Releases 2 Wiki Activity

Clean up the outline view #19

Merged
glen merged 23 commits from outline-cleanup_on_main into main 2024-11-15 03:32:48 +00:00
Conversation 9 Commits 23 Files Changed 7 +10 -10
2 changed files with 10 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 882286c0e3 - Show all commits

16
app-proto/main.css
View File

@ -61,12 +61,12 @@ summary.selected {
background-color: #444;
}
summary > div, .cst {
summary > div, .constraint {
padding-top: 4px;
padding-bottom: 4px;
}
.elt, .cst {
.elt, .constraint {
display: flex;
flex-grow: 1;
padding-left: 8px;
@ -91,7 +91,7 @@ details[open]:has(li) .elt-switch::after {
flex-grow: 1;
}
.cst-label {
.constraint-label {
flex-grow: 1;
}
@ -107,26 +107,26 @@ details[open]:has(li) .elt-switch::after {
width: 56px;
}
.cst {
.constraint {
font-style: italic;
}
.cst.invalid {
.constraint.invalid {
glen marked this conversation as resolved Outdated
glen commented 2024-11-13 17:45:25 +00:00
Outdated
Review
Copy Link

CSS class names, like variable names, should ideally be self-documenting. The class .cst doesn't say anything to me. Ideally, choose a different class name with clearer semantics. If that's not feasible for some reason, make sure to clearly comment the first use of the .cst class.

CSS class names, like variable names, should ideally be self-documenting. The class `.cst` doesn't say anything to me. Ideally, choose a different class name with clearer semantics. If that's not feasible for some reason, make sure to clearly comment the first use of the `.cst` class.
👍 1
Vectornaut commented 2024-11-14 00:42:33 +00:00
Outdated
Review
Copy Link

Done! I've expanded "cst" to "constraint," "elt" to "element," and "rep" to "representation" in the HTML element classes (commits 882286c and 3f3c173).

Done! I've expanded "cst" to "constraint," "elt" to "element," and "rep" to "representation" in the HTML element classes (commits 882286c and 3f3c173).
glen commented 2024-11-14 01:30:57 +00:00
Outdated
Review
Copy Link

Yes, much more intelligible! Thanks, resolving.

Yes, much more intelligible! Thanks, resolving.
color: #f58fc2;
glen marked this conversation as resolved Outdated
glen commented 2024-11-13 17:49:41 +00:00
Outdated
Review
Copy Link

I don't really mind #222 and #f0f0f0 sort of thing for various greys, because they are pretty easy to understand, but my mental hex-to-hue converter is otherwise not particularly good. Colors also often carry semantic content and/or play specific roles. So please don't use non-grey bare hex colors in css. Give them symbolic names that convey role (presuming there is one) and/or hue as well if feasible (or if not, comment the hue where the symbol is defined) and then use them symbolically, presumably with var(...) CSS references. For example, in Numberscope several colors are defined in App.vue and then used throughout the components with var(...) references. Thanks.

I don't really mind `#222` and `#f0f0f0` sort of thing for various greys, because they are pretty easy to understand, but my mental hex-to-hue converter is otherwise not particularly good. Colors also often carry semantic content and/or play specific roles. So please don't use non-grey bare hex colors in css. Give them symbolic names that convey role (presuming there is one) and/or hue as well if feasible (or if not, comment the hue where the symbol is defined) and then use them symbolically, presumably with `var(...)` CSS references. For example, in Numberscope several colors are defined in App.vue and then used throughout the components with `var(...)` references. Thanks.
👍 1
Vectornaut commented 2024-11-14 08:31:38 +00:00
Outdated
Review
Copy Link

Done (in commit 0c8022d). I've named the color variables according to their roles, with cues about the intended color limited to brightness descriptions. There are comments describing the colors on the lines where the color variables are defined. I'm hoping that this will make it easier to keep the color descriptions up to date with the actual colors.

Done (in commit 0c8022d). I've named the color variables according to their roles, with cues about the intended color limited to brightness descriptions. There are comments describing the colors on the lines where the color variables are defined. I'm hoping that this will make it easier to keep the color descriptions up to date with the actual colors.
glen commented 2024-11-14 15:45:36 +00:00
Outdated
Review
Copy Link

Those are excellent symbolic names and comments. If we are ever pulling in an external package that has css variables of its own, we will need to add a prefix (e.g., --text-bright becomes --dyna-text-bright), but they're good for now.

Those are excellent symbolic names and comments. If we are ever pulling in an external package that has css variables of its own, we will need to add a prefix (e.g., --text-bright becomes --dyna-text-bright), but they're good for now.
}
.cst > input[type=checkbox] {
.constraint > input[type=checkbox] {
margin: 0px 8px 0px 0px;
}
.cst > input[type=text] {
.constraint > input[type=text] {
color: inherit;
background-color: inherit;
border: 1px solid #555;
border-radius: 2px;
}
.cst.invalid > input[type=text] {
.constraint.invalid > input[type=text] {
border-color: #70495c;
}

4
app-proto/src/outline.rs
View File

@ -44,12 +44,12 @@ fn ConstraintOutlineItem(constraint_key: ConstraintKey, element_key: ElementKey)
};
let other_subject_label = assembly.elements.with(|elts| elts[other_subject].label.clone());
let class = constraint.lorentz_prod_valid.map(
|&lorentz_prod_valid| if lorentz_prod_valid { "cst" } else { "cst invalid" }
|&lorentz_prod_valid| if lorentz_prod_valid { "constraint" } else { "constraint invalid" }
);
view! {
li(class=class.get()) {
input(r#type="checkbox", bind:checked=constraint.active)
div(class="cst-label") { (other_subject_label) }
div(class="constraint-label") { (other_subject_label) }
LorentzProductInput(constraint=constraint)
div(class="status")
}