Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLDR-17799 Use Vue components for forum form and buttons #3968

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

btangmu
Copy link
Member

@btangmu btangmu commented Aug 18, 2024

-New ForumButton.vue, ForumForm.vue; revise cldrForum.mjs; related refactoring

CLDR-17799

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-New ForumButton.vue, ForumForm.vue; revise cldrForum.mjs; related refactoring
/**
* Encapsulate this class name -- caution: it's used literally in surveytool.css
*/
const FORUM_DIV_CLASS = "forumDiv";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FORUM_DIV_CLASS is moved from cldrForum to cldrForumPanel since no longer needs export

if (!window.getSelection) {
return false;
if (cldrForum.isFormVisible()) {
return true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this condition to isInputBusy is not elegant. It reflects the fact that we have Vue components (buttons) embedded in legacy DOM elements in the Info Panel, and the whole form is attached to one of those buttons and would get obliterated by an auto-scheduled page update if we didn't prevent it

@@ -0,0 +1,104 @@
<script setup>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"script setup" appears to be the latest recommended syntax, and so is putting script, template, style in that order

See:

https://vuejs.org/api/sfc-script-setup
https://vuejs.org/style-guide/rules-recommended#single-file-component-top-level-element-order

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's very handy, has some caveats.

@btangmu btangmu requested a review from srl295 August 21, 2024 22:12
@btangmu btangmu marked this pull request as ready for review August 21, 2024 22:12

function formatParent(parentPost) {
const div = cldrForum.parseContent([parentPost], "parent");
return new XMLSerializer().serializeToString(div);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method takes DOM elements generated by legacy code (cldrForum.parseContent) and turns them into html using v-html. In the short term this is easy and achieves consistency with the legacy formatting. Eventually of course there should be a Vue component for rendering completed forum posts.

);
const message =
"Your post was added, #" + data.postId + " but could not be shown.";
cldrNotify.error("Error posting to Forum", message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better

return false;
});
return replyButton;
function addPostVueButton(containerEl, pi, label, disabled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good incremental improvement. we'll get it all sorted out…

Comment on lines +458 to +463
/**
* An empty paragraph at the bottom of the info panel enables scrolling
* to bring the bottom content fully into view without being overlapped
* by the xpath shown by addXpath
*/
function addBottom() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be padding-bottom: 2em; or something in the CSS for the element instead? that would have a similar result without requiring mutation of the html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quick and avoids going deep into the legacy css. The order of elements in the info panel seems likely to change. Having this new "bottom" element at the bottom keeps its style separate from the rest.

@btangmu btangmu merged commit 1784c0c into unicode-org:main Aug 24, 2024
14 checks passed
@btangmu btangmu deleted the t17799_f branch August 24, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants