CLDR-16750 Add box gets stuck open, modernize #4066
Conversation
-New AddValue.vue and cldrAddValue.mjs -New NotificationPopup.vue, for new method cldrNotify.openWithHtml handling HTML in http responses -Add a-modal for Modal from Ant Design Vue, used by AddValue.vue and NotificationPopup.vue -Revise isInputBusy in cldrSurvey to inhibit table refresh by checking cldrAddValue.isFormVisible instead of popover-content -Repurpose the style tr_submit to highlight a row when a dialog is open for adding a new value -Remove dead code -Comments
|
Thanks for adding comments and cleaning up the code while modernizing components. Looks great -- is there anything else you plan to do? I see this is marked as "Draft". Does this change any of the visual appearances? |
Yes. The popup dialog for entering a new value is from a completely different library (Ant Design Vue instead of a decade-old version of Bootstrap) and it is different in style as well as behavior. One bug that it fixes is that it never goes "off the right hand side of the screen" (partially invisible) as described in the ticket. It's modern and behaves more like one would expect. Also it disappears if you click outside it. A screenshot of the old dialog is in the ticket. I'll post a new screenshot shortly. This is still a "draft" PR since I want to test a little more before making it ready for review... |
|
Screenshot of input dialog box based on this PR: Screenshot of error notification (near top-right corner of window, like other notifications) based on this PR: |
|
I've made this ready for review, but it is for v47 and merging can be postponed until we're ready to merge v47 changes into the main branch (or until we have a different branch for v47 changes) |
| const description = cldrSurvey.testsToHtml(tests); | ||
| cldrNotify.openWithHtml("Response to voting", description); |
There was a problem hiding this comment.
It'd be cleaner here (and avoid HTML injection issues?) by just opening a Vue form here that takes {tests} as a property and then displays the test in a structured way.
There was a problem hiding this comment.
yes... I need to study the server response more to see what it can contain... for this PR I didn't want to go too far all at once
| formTop.value = event.clientY - event.offsetY; | ||
| newValue.value = ""; | ||
| formIsVisible.value = true; | ||
| cldrAddValue.setFormIsVisible(true, xpstrid.value); |
There was a problem hiding this comment.
"form is visible" would be ideally kept in the presentation (vue) layer rather than owned by the "library". Couldn't it be a property on this AddValue component? Maybe for a future refactor.
There was a problem hiding this comment.
yes...
the overall situation is that we have vue components embedded in legacy components, and the latter can get clobbered when a response arrives
setFormIsVisible enables cldrSurvey.isInputBusy to prevent that disruption by checking cldrAddValue.isFormVisible
if we moved that into vue, then cldrSurvey would need to know about vue -- problematic for encapsulation and further mixing of modern/legacy code
long-term solution is more extensive modernization; in the meantime there are compromises
| style="top: 24px; right: 24px; position: absolute" | ||
| @ok="onClose" | ||
| > | ||
| <div v-html="description" /> |
There was a problem hiding this comment.
🙈
I'm OK with this for now, if we file a ticket (and add a TODO) to fix this as new tech debt…
There was a problem hiding this comment.
We have so many techdebt issues with "minor" priority already... but OK, I'll make another one :-)
I'm not sure how specific or general it should be. A general issue is that the back end often includes HTML in http responses. If I remember right, this v-html is an example of handling html that comes from an http response, though it's further complicated by cldrSurvey.testsToHtml adding further html...
I just made this ticket: https://unicode-org.atlassian.net/browse/CLDR-18013
There was a problem hiding this comment.
I'll follow up with a PR adding a TODO referencing that ticket
-New AddValue.vue and cldrAddValue.mjs
-New NotificationPopup.vue, for new method cldrNotify.openWithHtml handling HTML in http responses
-Add a-modal for Modal from Ant Design Vue, used by AddValue.vue and NotificationPopup.vue
-Revise isInputBusy in cldrSurvey to inhibit table refresh by checking cldrAddValue.isFormVisible instead of popover-content
-Repurpose the style tr_submit to highlight a row when a dialog is open for adding a new value
-Remove dead code
-Comments
CLDR-16750
ALLOW_MANY_COMMITS=true