Visual feedback about pagination status #251
Conversation
There was a problem hiding this comment.
Thank you for the contribution, I really appreciate it!
I hadn't realized how poor the state of navigating SCM has gotten -- pressing the "next" page after loading seemed to consistency fail for multiple tries without any extensions.
I've seen the extension receive 1 star reviews by users thinking the extension introduced this issue, so yeah, I'm thinking about how to integrate messaging in the status bar that indicates the request to Steam failed.
| }, 1000); | ||
| } | ||
|
|
||
| class PaginationFailedBox extends HTMLElement { |
There was a problem hiding this comment.
Would it be possible for you to define this as a FloatElement that uses Lit?
You get a couple nice perks for that:
- Event handling (ie. click events) that have nicer syntactic sugar
- Render loop handling (prevents having to perform manual add/remove styles, changing the rendered HTML)
- Follows the convention for creating components across the code-base.
You can see other components in this parent folder for examples.
| } | ||
| } | ||
|
|
||
| customElements.define('pagination-failed-box', PaginationFailedBox); |
There was a problem hiding this comment.
Preferable to use FloatElement with the decorator so that this happens for you.
|
|
||
| customElements.define('pagination-failed-box', PaginationFailedBox); | ||
| const paginationBox = document.createElement('pagination-failed-box') as PaginationFailedBox; | ||
| document.body.appendChild(paginationBox); |
There was a problem hiding this comment.
I'd prefer a decorator (perhaps in the family of @InjectX) -- perhaps call it InjectBody()? There may also be an element on the page that may make more sense to inject into (perhaps closer to the results page).
| } | ||
|
|
||
| export function initResponseRenderResultsStatus() { | ||
| const id = setInterval(() => { |
There was a problem hiding this comment.
I think it'd be better to get something like this abstracted like Observe in utils.
For instance, WaitUntilDefined and make it a bit more generic.
| @@ -0,0 +1,155 @@ | |||
| function overrideOnResponseRenderResults(proto: any) { | |||
There was a problem hiding this comment.
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.
|
|
||
| proto.OnResponseRenderResults = function(responseClass: any) { | ||
| const success = (responseClass && responseClass.responseJSON && responseClass.responseJSON.success) || false; | ||
| if (!success) { |
There was a problem hiding this comment.
nit: Anti-pattern to do:
if (!var) {
// this
} else {
// that
}
instead of
if (var) {
// that
} else {
// this
}
| 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.
| const success = (responseClass && responseClass.responseJSON && responseClass.responseJSON.success) || false; | |
| const success = !!responseClass?.responseJSON?.success; |
| }, 1000); | ||
| } | ||
|
|
||
| class PaginationFailedBox extends HTMLElement { |
There was a problem hiding this comment.
PaginationFailedBox seems to be a bit of a misleading term since it also showing "pending" status.
How about PaginationStatusBox?
|
|
||
| init('src/lib/page_scripts/market_listing.js', main); | ||
|
|
||
| async function main() {} | ||
| async function main() { | ||
| initResponseRenderResultsStatus() |
There was a problem hiding this comment.
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.
| private progressBar: HTMLDivElement; | ||
| private messageDiv: HTMLDivElement; | ||
| private hideTimeout: number | null = null; | ||
| private timeoutDuration: number = 1; |
There was a problem hiding this comment.
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. timeoutMs) and use it consistently.
Hi, I couldn't stand using market without understanding the status of pagination request so I made this small visualization
demo-status.mp4