Closed benloh closed 1 year ago
In GitLab by @daveseah on May 15, 2019, 12:43
marked as a Work In Progress
In GitLab by @daveseah on May 15, 2019, 12:44
Added WIP: to the title to prevent merges by GitLab, as this is stated in the original description.
In GitLab by @daveseah on May 15, 2019, 12:44
The test loop works for me. Going to do a code review next as comments so we capture everything in one place.
In GitLab by @daveseah on May 15, 2019, 12:49
Code Review Process. Going to try asking these questions and see if the answers help formulate something useful...
In GitLab by @daveseah on May 15, 2019, 14:43
0. FILE BY FILE SCAN writing things I'm noting as I go through each change, using SourceTree branch diff between latest branch a62a3714 and dev-bl/input-ui 8dd69c50
POSSIBLE TECHNICAL DEBT
ViewMain
selectedResource.links
property initializes with a guard value (make sure this isn't fragile)document.getElementById()
to read form element values (this seems unportable and not very REACT-like)PMCData
PMCView
Class VPROP
ResourceItem
Collapse
replace Unexpand
as a term?isExpanded
property raises question: modeling a component with internal state named semantically, not stylistically/mechanically, for better readability. This is a general challenge when integrating code from different sourcesSET_EVIDENCE_LINK_WAIT_FOR_SOURCE_SELECT
message name is super long and implement a TRANSACTION, when messages might not be a suitable way to model application state. This will introduce all kinds of fragility in the application, so we need to rewrite this when the UR.EXEC and UR.MODESET modules formalize how to do this.SHOW_
messages that are used to directly trigger UI behavior rather than dataflow is bad practice because it uses async methods without synchronization with our data-driven view architecture. This is a potential source of render bugs (such as the 'resource closed before selection' bug). EvidenceLink
SET_EVIDENCE_LINK_WAIT_FOR_SOURCE_SELECT
is also used in ResourceItem
, which is fragile coupling to coordinate components. The SELECTION_CHANGE
message appears to be related to this initial call as part of a TRANSACTION; the name of the message doesn't indicate these messages are related on top of it being a bad practice. We had code like this in early STEP code across the network and it made it VERY DIFFICULT to add features without breaking the code in terrible, difficult-to-trace ways. This has to be excised before we continue with the React UI. EvidenceList
handleDataUpdate()
interesting note on this.forceUpdate()
requiring that evlink.note updates aren't being updated as user types. Wondering why evlink itself just doesn't update locally through its own on-change handlerResourceList
?General
In GitLab by @daveseah on May 15, 2019, 14:54
Skipping steps 1-3 since step 0 has familiarized myself with the general shape of the code. Now moving on to step 4 next, but I think that will be part of the next design pass because of the message and asynch/synch issues I see. I think adding mechanisms for handling transactions and UX action paths will help resolve that.
added 14 commits
Some responses:
ViewMain
- state
selectedResource.links
property initializes with a guard value (make sure this isn't fragile)
The intent was to initialize the object with the proper type. links
is a calculated value, but just used for display.
- use of
document.getElementById()
to read form element values (this seems unportable and not very REACT-like)
To keep the focus on quickly hacking out a UI for the pilot, I took some shortcuts rather than writing a state handler for component when we weren't even sure what we needed to build.
See Merge Request 9 https://gitlab.com/inq-seeds/boilerplate/merge_requests/9 and the dev-bl/viewmain-refactor
branch for a few fixes along these lines.
ResourceItem
- Can
Collapse
replaceUnexpand
as a term?
Done. See a9131792d914e5e94963cc9565db5d5c69c9dc11 in dev-bl/resourceitem-refactor
- COMMENT: Naming of
isExpanded
property raises question: modeling a component with internal state named semantically, not stylistically/mechanically, for better readability. This is a general challenge when integrating code from different sources
Any suggestions?
- 'resource closed before selection' bug is a sign of synchronous fighting asynchronous code
Haven't seen this.
SET_EVIDENCE_LINK_WAIT_FOR_SOURCE_SELECT
message name is super long and implement a TRANSACTION, when messages might not be a suitable way to model application state. This will introduce all kinds of fragility in the application, so we need to rewrite this when the UR.EXEC and UR.MODESET modules formalize how to do this.
Will leave this alone for now until we have a revised URSYS.
- The definition and use of
SHOW_
messages that are used to directly trigger UI behavior rather than dataflow is bad practice because it uses async methods without synchronization with our data-driven view architecture. This is a potential source of render bugs (such as the 'resource closed before selection' bug).
Right now opening and closing (actually expanding and collapsing) of resource items is handled via a local state variable triggered by an URSYS call. If this needs to be handled via dataflow, doesn't this mean we need a separate global data object that is maintaining UI state for all components?
EvidenceLink
SET_EVIDENCE_LINK_WAIT_FOR_SOURCE_SELECT
is also used inResourceItem
, which is fragile coupling to coordinate components. TheSELECTION_CHANGE
message appears to be related to this initial call as part of a TRANSACTION; the name of the message doesn't indicate these messages are related on top of it being a bad practice. We had code like this in early STEP code across the network and it made it VERY DIFFICULT to add features without breaking the code in terrible, difficult-to-trace ways. This has to be excised before we continue with the React UI.
This is related to the issue of needing to maintain UI state apart from data. Though we might have a new method...
EvidenceList
handleDataUpdate()
interesting note onthis.forceUpdate()
requiring that evlink.note updates aren't being updated as user types. Wondering why evlink itself just doesn't update locally through its own on-change handler
This is now implemented via a refactor. See branch dev-bl/evlink-note-refactor
, or Merge Request #8 https://gitlab.com/inq-seeds/boilerplate/merge_requests/8
- Shouldn't this be called
ResourceList
?
No because this is a list of EvidenceLinks. If anything it should be called EvidenceLinkList. The Resource Library could be called the Resource List.
General
- Conform our methods to PascalCase method names just so our methods stand out from libraries.
Will start doing this with refactoring.
- It looks like React components may not have been renamed to reflect "ResourceList" being the new terminology.
I think this has been done already. It might be a misunderstanding about the difference between a EvidenceList and the ResourceList (actually the Resource Library component in ViewMain). But if I missed something, please note it.
added 3 commits
In GitLab by @daveseah on May 17, 2019, 04:13
Cool, looking forward to seeing the refactor!
Takeaways from your comments on my comments:
A few quick responses
If this needs to be handled via dataflow, doesn't this mean we need a separate global data object that is maintaining UI state for all components?
Yah, that is the ViewModel stuff that's temporarily stored in PMCData at this point, but ideally we make something that is designed to work both top-down and bottom-up styles of application prototyping.
Perhaps we'll do a system design workshop and try defining a system together that is similar to this app so we develop a common vocabulary and grammar for expressing such designs.
Maybe an easier way to think about it is to try to tag every variable name, message, and function name so it's possible to know which system is affected, and whether the operation affects data, view, logic, or is a transformation of some kind. Ideally, the collection of all these names should evoke a mental model of the broad system just by reading them, rather than a point-to-point spaghetti-like network that is hard to analyze.
Likewise, these names ideally are fairly short, consistent in their grammatical construction, and reflect the way the action is scheduled. There's a rich vocabulary that we could further define.
I do like the idea of a system design workshop. Maybe that will be our next code camp junket!
added 8 commits
unmarked as a Work In Progress
merged
mentioned in commit 2d2645063a22ff16b0c9ece241dd2c5b55c2e1be
Merges dev-bl/badge-mgr -> dev-bl/input-ui
[Please do not merge this yet! We want to wait for the initial pilot to finish as well as fix the layout bug bug Issue #4 before merging.]
Evidence Badge Manager
An Evidence Badge is displayed on a Component to indicate that there is an Evidence Link that has been created between a Resource Item in the Resource Library and that Component. e.g. A student might add a link between the Resource Item "(1) Food Rot Simulation" and the component "Ammonia". A "(1)" badge is then displayed on the Ammonia Component.
This re-implements Evidence Badges by adding a Badge Management function within
class-vprop.js
. It monitors Evidence Links and adds, removes, and updates Badges when links are added, removed, and updated.(The old Evidence Badge system was flawed in that we were hacking in badges during the VProp.Update and VProp.Draw calls, resulting in duplicate badges being created and caused unreliable click handling).
The Cycle
It is perhaps easiest to understand how it functions by following a typical update cycle.
SVGView.DoAppLoop
First, we monitor and update Evidence Links in the SVGView.DoAppLoop, adding a
PMCVIew.SyncBadgesaFromEvLinkData()
call.pmc-view.SyncBadgesFromEvLinkData
Using a similar technique to PMCView.SyncPropsFromGraphData, we first request the
added
,removed
, andupdated
evidence links frompmc-data.VM_GetVBadgeChanges()
.pmc-data.VM_GetVBadgeChanges()
Here we just walk down the
a_evidence
array of Evidence Links, keeping track of added and removed items. This is passed back to pmc-view.SyncBadgesFromEvLinkData.pmc-view.SyncBadgesFromEvLinkData
Back to SyncBadgesFromEvLinkData...We review the evidence links, new and old, and update the vprop badges based on the changes.
For new evidence links, we create a new VProp badge by calling VProp.NewBadge. Removed badges call VProp.ReleaseBadge() Updated badges call VProp.UpdateBadge().
class-vprop.NewBadge
Rather than creating a whole new visual manager to handle badges, it seemed to make sense to add additional functionality to class-vprop. (The badges needed to be drawn with the parent component, and since badges for mechanisms would require a different drawing mechanism anyway, it seemed like there wasn't a need to separate it out -- it's worth revisiting though).
VProp.NewBadge
is modeled after the VProp constructor: we create a circle and text element tied ot thegRoot
object, add aRelease()
andUpdate()
methods, and set the initial drawing parameters. We also attach click listeners to open the parent Evidence Links via URSYS calls.VProp.ReleaseBadge
andVProp.UpdateBadge
handle the remove and update calls frompmc-view.SyncBadgesFromEvLinkData
.To Test
The evidence badge should appear on the Component you created.
Issues
There is a bug in the layout where newly added badges seem to bump the origin of the the parent Component by about 20 pixels. See Issue #4.
The badges don't draw until you click on the graph. Do we need to force an update call somewhere?
Should this functionality be handled by a separate v-class or is it OK to be handled within
class-vprop
?Similarly, since we also need to build a badge manager for
class-vmech
, wouldclass-vprop
andclass-vmech
be able to share a common badge class? Or are they different enough that they should have separate badge managers (I couldn't figure out how to attach a circle badge to the textpath).