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.

Sign up for GitHub

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

Jump to bottom

bugfix: Add ability to reset mini #25

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

bugfix: Add ability to reset mini #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
41c5756
bugfix: Add ability to reset mini
notthatjen Jun 8, 2024
0ead842
add each child template into a template tag
notthatjen Jun 8, 2024
e930d41
Remove entity disposal during reset
notthatjen Jun 9, 2024
faaaa70
Revert memoize for state.window
notthatjen Jun 9, 2024
b264c14
Reset minijs when user navigate back
notthatjen Jun 9, 2024
7b00afb
Add debounce time on event listeners
notthatjen Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,7 @@ <h3 class="font-bold font-mono">On Change Example:</h3>
<script>
showSelect = false
</script>
<div class="p-4 border border-dashed border-black rounded mt-2">
<div class="p-4 border border-dashed border-black rounded mt-2" :clickout="showSelect = false">
<div class="flex items-end">
<div class="flex flex-col w-full">
<p class="font-bold font-mono text-sm">Custom Select:</p>
Expand Down Expand Up @@ -1107,6 +1107,35 @@ <h3 class="font-bold font-mono">On Change Example:</h3>
</svg>
</span>
</li>
<li
:click="selectedOption = 'Dwight Schrute'"
:class="selectedOption == 'Dwight Schrute' ? 'bg-indigo-600 text-white' : 'text-gray-900' "
class="text-gray-900 relative cursor-default select-none py-2 pl-3 pr-9"
id="listbox-option-0"
role="option"
>
<span
:class="selectedOption == 'Dwight Schrute' ? 'font-semibold' : 'font-normal' "
class="block truncate"
>Dwight Schrute</span
>
<span
class="text-white absolute inset-y-0 right-0 flex items-center pr-4"
>
<svg
class="h-5 w-5"
viewBox="0 0 20 20"
fill="currentColor"
aria-hidden="true"
>
<path
fill-rule="evenodd"
d="M16.704 4.153a.75.75 0 01.143 1.052l-8 10.5a.75.75 0 01-1.127.075l-4.5-4.5a.75.75 0 011.06-1.06l3.894 3.893 7.48-9.817a.75.75 0 011.05-.143z"
clip-rule="evenodd"
/>
</svg>
</span>
Comment on lines +1111 to +1137
Copy link

Choose a reason for hiding this comment

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

Ensure consistent ID attributes for list items.

The id="listbox-option-0" is repeated across multiple list items. This can lead to invalid HTML as id attributes must be unique within a page. Consider using a dynamic value or removing these IDs if they are not necessary.

- id="listbox-option-0"
+ id="listbox-option-{{ index }}"

Committable suggestion was skipped due to low confidence.

</li>
<li
:click="selectedOption = 'Jen Villaganas'"
:class="selectedOption == 'Jen Villaganas' ? 'bg-indigo-600 text-white' : 'text-gray-900' "
Expand Down
18 changes: 18 additions & 0 deletions lib/entity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ export class Entity {
return document.documentElement.contains(this.element)
}

isEachEl() {
return this.element.hasAttribute(':each')
}

isInsideEachEl() {
if (this.element.hasAttribute(':each')) return false
if (this.element.hasAttribute(':each.item')) return true
Expand Down Expand Up @@ -80,6 +84,20 @@ export class Entity {
)
}

setEachItemTemplate() {
if (this.getEachItemTemplate()) return
const template = document.createElement('template')
template.innerHTML = this.element.innerHTML
template.id = "ChildTemplate:" + this.id
document.body.appendChild(template)
this.element.dataset.templateId = template.id
}

getEachItemTemplate() {
const template = document.getElementById(this.element.dataset.templateId)
return template?.content
}

getClosestEl(attr) {
attr = attr.replaceAll(':', '\\:').replaceAll('.', '\\.')

Expand Down
3 changes: 2 additions & 1 deletion lib/entity/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,8 @@ export class Attributes {

try {
const items = await this._interpret(iterable)
this.childClone ||= this.entity.element.cloneNode(true).children
this.entity.setEachItemTemplate()
this.childClone ||= this.entity.getEachItemTemplate().children

this.entity.element.innerHTML = ''

Expand Down
22 changes: 15 additions & 7 deletions lib/entity/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,16 +149,24 @@ export class Events {
const listener = this.listener[attr]
if (!listener) return

if (Array.isArray(listener)) {
listener.forEach(({ el, eventName, event }) => {
el.addEventListener(eventName, event)
[].concat(listener).forEach(({ el, eventName, event }) => {
el.addEventListener(eventName, (e) => {
this.debounce(() => {
event(e)
}, 50)
})
} else {
const { el, eventName, event } = listener
el.addEventListener(eventName, event)
}
})
Comment on lines +152 to +158
Copy link

Choose a reason for hiding this comment

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

Improve the implementation of applyEvent method by simplifying the debounce usage.

-    [].concat(listener).forEach(({ el, eventName, event }) => {
-      el.addEventListener(eventName, (e) => {
-        this.debounce(() => {
-          event(e)
-        }, 50)
-      })
-    })
+    listener.forEach(({ el, eventName, event }) => {
+      el.addEventListener(eventName, this.debounce(event, 50))
+    })

This change ensures that the event handler itself is debounced, rather than creating a new debounced function every time an event is triggered, which is more efficient and cleaner.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[].concat(listener).forEach(({ el, eventName, event }) => {
el.addEventListener(eventName, (e) => {
this.debounce(() => {
event(e)
}, 50)
})
} else {
const { el, eventName, event } = listener
el.addEventListener(eventName, event)
}
})
listener.forEach(({ el, eventName, event }) => {
el.addEventListener(eventName, this.debounce(event, 50))
})

}

debounce(callback, wait) {
if (this.timeout) return
clearTimeout(this.timeout)
this.timeout = setTimeout(() => {
callback()
this.timeout = null
}, wait)
}
Comment on lines +161 to +168
Copy link

Choose a reason for hiding this comment

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

Refactor the debounce method to prevent multiple simultaneous timeouts.

-    if (this.timeout) return
-    clearTimeout(this.timeout)
+    clearTimeout(this.timeout);

This change ensures that any previous debounce timeouts are cleared before setting a new one, preventing multiple debounce operations from overlapping incorrectly.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
debounce(callback, wait) {
if (this.timeout) return
clearTimeout(this.timeout)
this.timeout = setTimeout(() => {
callback()
this.timeout = null
}, wait)
}
debounce(callback, wait) {
clearTimeout(this.timeout);
this.timeout = setTimeout(() => {
callback()
this.timeout = null
}, wait)
}


setChangeEvent() {
const el = this.entity.element
const key = ':change'
Expand Down
16 changes: 16 additions & 0 deletions lib/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ export class Mini {
console.log(`MiniJS took ${executionTime}ms to run.`)
}

async reset() {
this.observer.disconnect()
this.state.dispose()
await this.init()
}
Comment on lines +45 to +49
Copy link

Choose a reason for hiding this comment

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

Ensure error handling for the init method in the reset function.

- await this.init()
+ try {
+   await this.init()
+ } catch (error) {
+   console.error('Error during re-initialization:', error);
+ }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async reset() {
this.observer.disconnect()
this.state.dispose()
await this.init()
}
async reset() {
this.observer.disconnect()
this.state.dispose()
try {
await this.init()
} catch (error) {
console.error('Error during re-initialization:', error);
}
}


async _domReady() {
return new Promise((resolve) => {
if (document.readyState == 'loading') {
Expand Down Expand Up @@ -86,6 +92,13 @@ const MiniJS = (() => {
console.error('Error initializing MiniJS:', error)
})

// Reset MiniJS when the user navigates back
window.addEventListener('popstate', () => {
mini.reset().catch((error) => {
console.error('Error resetting MiniJS after navigation:', error);
});
});

return {
get debug() {
return Mini.debug
Expand All @@ -108,6 +121,9 @@ const MiniJS = (() => {
extendEvents: (events) => {
mini.extensions.events.extend(events)
},
reset: async () => {
await mini.reset()
}
}
})()

Expand Down
7 changes: 7 additions & 0 deletions lib/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,4 +297,11 @@ export class State {
const key = `${entityID}.${variable}`
this.entityVariables.delete(key)
}

dispose() {
this.shouldUpdate = false
this.entities.clear()
this.entityVariables.clear()
this.variables.clear()
}
}
Loading