-
Notifications
You must be signed in to change notification settings - Fork 30
Visual feedback about pagination status #251
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,155 @@ | ||||||
function overrideOnResponseRenderResults(proto: any) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please define types for |
||||||
const originalMethod = proto.OnResponseRenderResults; | ||||||
|
||||||
proto.OnResponseRenderResults = function(responseClass: any) { | ||||||
const success = (responseClass && responseClass.responseJSON && responseClass.responseJSON.success) || false; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if (!success) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Anti-pattern to do:
instead of
|
||||||
paginationBox.showError(); | ||||||
} else { | ||||||
paginationBox.hide(); | ||||||
} | ||||||
|
||||||
originalMethod.call(this, responseClass); | ||||||
}; | ||||||
} | ||||||
|
||||||
function overrideGoToPage(proto: any) { | ||||||
const originalMethodGoToPage = proto.GoToPage; | ||||||
|
||||||
proto.GoToPage = function(page: number) { | ||||||
paginationBox.showPending(); | ||||||
originalMethodGoToPage.call(this, page); | ||||||
}; | ||||||
} | ||||||
|
||||||
export function initResponseRenderResultsStatus() { | ||||||
const id = setInterval(() => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be better to get something like this abstracted like For instance, |
||||||
const searchResultsObject = g_oSearchResults; | ||||||
if (!searchResultsObject) return; | ||||||
|
||||||
const proto = Object.getPrototypeOf(searchResultsObject); | ||||||
|
||||||
overrideOnResponseRenderResults(proto); | ||||||
overrideGoToPage(proto); | ||||||
|
||||||
clearInterval(id); | ||||||
}, 1000); | ||||||
} | ||||||
|
||||||
class PaginationFailedBox extends HTMLElement { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible for you to define this as a You get a couple nice perks for that:
You can see other components in this parent folder for examples. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How about |
||||||
private container: HTMLDivElement; | ||||||
private progressBar: HTMLDivElement; | ||||||
private messageDiv: HTMLDivElement; | ||||||
private hideTimeout: number | null = null; | ||||||
private timeoutDuration: number = 1; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has no clear units, for example, it seems to represent 5s increments for showing pending and seconds for error. You should put the units in the variable name (ie. |
||||||
private state: string = 'none'; | ||||||
|
||||||
constructor() { | ||||||
super(); | ||||||
this.container = document.createElement('div'); | ||||||
this.messageDiv = document.createElement('div'); | ||||||
this.progressBar = document.createElement('div'); | ||||||
this.attachShadow({ mode: 'open' }); | ||||||
this.createElements(); | ||||||
this.setupStyles(); | ||||||
this.addEventListeners(); | ||||||
} | ||||||
|
||||||
private createElements() { | ||||||
this.container.appendChild(this.messageDiv); | ||||||
this.container.appendChild(this.progressBar); | ||||||
this.shadowRoot!.appendChild(this.container); | ||||||
} | ||||||
|
||||||
private setupStyles() { | ||||||
Object.assign(this.container.style, { | ||||||
position: 'fixed', | ||||||
top: '10px', | ||||||
right: '10px', | ||||||
padding: '10px', | ||||||
color: 'white', | ||||||
fontSize: '14px', | ||||||
borderRadius: '5px', | ||||||
zIndex: '1000', | ||||||
display: 'none', | ||||||
}); | ||||||
|
||||||
Object.assign(this.messageDiv.style, { | ||||||
position: 'relative', | ||||||
}); | ||||||
|
||||||
Object.assign(this.progressBar.style, { | ||||||
position: 'absolute', | ||||||
bottom: '0', | ||||||
left: '0', | ||||||
height: '2px', | ||||||
width: '100%', | ||||||
backgroundColor: 'white', | ||||||
transition: `width ${this.timeoutDuration}s linear`, | ||||||
}); | ||||||
} | ||||||
|
||||||
private addEventListeners() { | ||||||
this.container.addEventListener('click', () => { | ||||||
this.hide(); | ||||||
}); | ||||||
} | ||||||
|
||||||
showError(duration: number = this.timeoutDuration) { | ||||||
this.state = 'error'; | ||||||
this.timeoutDuration = duration; | ||||||
this.messageDiv.textContent = 'Pagination failed'; | ||||||
this.container.style.backgroundColor = 'red'; | ||||||
this.container.style.display = 'block'; | ||||||
this.progressBar.style.width = '100%'; | ||||||
this.progressBar.style.transitionDuration = `${this.timeoutDuration}s`; | ||||||
|
||||||
if (this.hideTimeout) { | ||||||
clearTimeout(this.hideTimeout); | ||||||
} | ||||||
|
||||||
setTimeout(() => { | ||||||
this.progressBar.style.width = '0%'; | ||||||
}, 10); | ||||||
|
||||||
this.hideTimeout = window.setTimeout(() => { | ||||||
this.hide(); | ||||||
}, this.timeoutDuration * 1000); | ||||||
} | ||||||
|
||||||
showPending() { | ||||||
this.state = 'pending'; | ||||||
this.messageDiv.textContent = 'Pending'; | ||||||
this.container.style.backgroundColor = 'lightblue'; | ||||||
this.container.style.display = 'block'; | ||||||
this.progressBar.style.width = '100%'; | ||||||
this.progressBar.style.transitionDuration = `${this.timeoutDuration * 5}s`; | ||||||
|
||||||
setTimeout(() => { | ||||||
this.progressBar.style.width = '0%'; | ||||||
}, 10); | ||||||
|
||||||
const id = setInterval(() => { | ||||||
if (this.state !== 'pending') { | ||||||
clearInterval(id); | ||||||
return; | ||||||
} | ||||||
this.progressBar.style.transitionDuration = '0s'; | ||||||
this.progressBar.style.width = '100%'; | ||||||
|
||||||
setTimeout(() => { | ||||||
this.progressBar.style.transitionDuration = `${this.timeoutDuration * 5}s`; | ||||||
this.progressBar.style.width = '0%'; | ||||||
}, 10); | ||||||
}, this.timeoutDuration * 1000 * 5); | ||||||
} | ||||||
|
||||||
hide() { | ||||||
this.container.style.display = 'none'; | ||||||
this.state = 'none'; | ||||||
} | ||||||
} | ||||||
|
||||||
customElements.define('pagination-failed-box', PaginationFailedBox); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preferable to use |
||||||
const paginationBox = document.createElement('pagination-failed-box') as PaginationFailedBox; | ||||||
document.body.appendChild(paginationBox); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer a decorator (perhaps in the family of |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,11 @@ | ||
import {init} from './utils'; | ||
import '../components/market/item_row_wrapper'; | ||
import '../components/market/utility_belt'; | ||
import '../components/market/response_render_results_status'; | ||
import {initResponseRenderResultsStatus} from '../components/market/response_render_results_status'; | ||
|
||
init('src/lib/page_scripts/market_listing.js', main); | ||
|
||
async function main() {} | ||
async function main() { | ||
initResponseRenderResultsStatus() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in another comment, it's preferrable for a decorator of the component to handle it's own injection into the page, so that you don't need to handle creation manually here. Then you just need to import the file. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions can be included as part of the inner impl of the component, specifically when it starts you can handle getting the proto reference and overriding. Since at the end of the day, it is tightly coupled anyways and isn't capable of operating outside of a global reference to that component.