Smithsonian / dpo-voyager

DPO Voyager - 3D Explorer and Tool Suite
Apache License 2.0
164 stars 28 forks source link

remove all references to ff-property-view and ff-property-field. Fix #257 #259

Closed sdumetz closed 2 months ago

sdumetz commented 4 months ago

So, following up on #257 was something of a rabbit hole.

TL:DR: Those changes removes ~1000 lines of hard-to-maintain code while providing the following :

Long version :

Turns out there were (kind of) 3 systems, none of them working in every case:

It felt like a good opportunity to delete (stop using, really: Can't delete across submodules.) a good thousand lines of code. The main reasoning here is PropertyField is too hard to read and tries to do too much using very specific code where native components do (mostly) what we want in many cases.

Where possible, custom components were replaced by their native counterparts. In particular:

It allows native support of things previously handled by internal code (less code, possible quality improvement):

I added some binding code to those elements to preserve useful features from the current implementation :

Due to the higher specificity of components, this code is generally shorter and easier to identify.

I intentionally removed some things (that can be added back if needed):

All in all, I think it's a better starting point for any UI changes we might want to do in the future than the existing system.

I tested I much as I could, on desktop Chrome/Firefox and iOS safari, everything should work as well/better than before!

gjcope commented 4 months ago

A lot to test here, but I like the direction. Thanks for the effort!

gjcope commented 3 months ago

@sdumetz looking to push this to my team for testing but a couple of issues on first review.

If you have any ideas on fixes for the styling issue with the new scheme that would be greatly appreciated.

gjcope commented 3 months ago

@sdumetz Fixed a couple more bugs with updating and slider input scaling (https://github.com/Smithsonian/dpo-voyager/tree/dev-ui-update)

Tried addressing the styling issues above but still looked weird. Any help appreciated before pushing this along. Thanks!

sdumetz commented 3 months ago

I just tested (from the dev-ui-update branch) the drag&drop on the annotation image property and it worked fine, except the fact it changes the line's box-sizing when adding the borders. What should I look for specifically?

Similarly, do you have an example of a select that now looks weird?

sdumetz commented 3 months ago

in source/client/ui/story/styles.scss:L531:

////////////////////////////////////////////////////////////////////////////////
// MISC
.sv-drop-zone {
  border-style: dashed;
  border-width: 2px;
  border-color: blue;
  &.sv-property-view{
    margin: -2px;
  }
}

Fixes the reflow issue with dragdrop on properties.

Not what you asked for, but it bothered me :wink:

gjcope commented 3 months ago

I just tested (from the dev-ui-update branch) the drag&drop on the annotation image property and it worked fine, except the fact it changes the line's box-sizing when adding the borders. What should I look for specifically?

Yes, sorry I was testing Audio. It does look like the image functionality is working but file path and caption path for audio elements are not. The dragging and dropping is fixed but the overlay border is getting overridden in the css flow. Also I see with the image one the it is outlining the entire element instead of just the input. Not necessarily bad, just seems a little odd.

Similarly, do you have an example of a select that now looks weird?

There are a lot of examples when inputs are given enough vertical space, but here's a screenshot

image

sdumetz commented 3 months ago

Oh, forgot to check those tabs, I was focusing too muchon the settings tab.

Either assigning a max-height: 1.75rem to .sv-property-view or something like this :

.sv-property-view {
  flex-grow: 1;
  .ff-flex-column &, sv-panel & {
  flex-grow: 0;
}
[...]

}

The max-height is probably better, I can't see a place where we wouldn't want this, instead of manually adding exceptions on every flex-direction: column containers.

Concerning the audio input, it's because the sv-drop-zone class is applied on the .sv-property-field while the annotation task assigns the class to it's parent (.sv-property-string).

One of them should probably be changed to be consistent across tabs?

If applying the drop-zone to the whole line, no more style change is needed (it seems to work fine if the file is dropped on the label, so it makes some sort of sense).

If applying it to the sv-property-field, it might be one of the few cases where the !important css property is actually desirable and the best tool for the job as I don't see a case where we would want those rules be superseded and tuning the CSS selector feels wrong here.

gjcope commented 3 months ago

Thanks for the help! I've added the max-height and took the route of changing the annotation image drag/drop to standardize. I originally tried the other direction to avoid use of !important, but it's too much of a mess. With the two dropzone elements in the audio tab being vertically next to each other, there is a margin collapse issue that causes events that oscillate back and forth between dragenter and drag leave when in the overlap.

It doesn't really bother me, but FYI it doesn't look like the proposed margin update fixed the reflow issue.