-
Notifications
You must be signed in to change notification settings - Fork 458
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
Fix the navigation_count function and selector #776
Conversation
Ive given the PR a test-run on a few different accounts:
Results below. Regular gmail.com:Unread-fix confirmed working. OG, grandfathered "Google Apps" account.Unread-fix confirmed working. Paid, company GSuite account:Unread-fix partially working. Better than before, but produces console-warnings for all counters not Inbox, and provides |
I see storage_info() is implemented using a regexp on text values. Below are the texts Ive seen for my accounts:
Based on the regexp used, Id say the reason for the failure is assuming "X of Y" format and assuming percentage at the end. Maybe you can relax the conditions slightly? |
@josteink thank you so much for testing! One big issue is that a lot of data isn't in the DOM until the page is fully loaded. After
Do you think it should return undefined instead? I didn't want to break any scripts out there using this.
Will do. |
What might help make things easier for you is to separate data-fetching from data-processing into 2 different functions. That way you can unit-test the processing and just add my real world examples above as test-cases. That may sound complicated at first, but I think it will help you solve this faster and better. |
@@ -338,7 +338,7 @@ var Gmail = function(localJQuery) { | |||
}; | |||
|
|||
api.dom.inbox_content = function() { | |||
return $("div[role=main]:first"); | |||
return $("div[role=main]:first") && $("[aria-label='Gmail']").length; |
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.
I dont understand this change. Previously the function would return a jQuery element (as formalized in the gmail.d.ts
typescript definitions), but now you are returning a number?
The documentation also clearly states that this function should return DOM content.
DOM
These methods return the DOM data itself
gmail.dom**.inboxes()**
gmail.dom**.inbox_content()**
So what's the idea behind this change?
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.
@josteink Opps! I didn't mean to push this to the PR. Actually, I'm just going to close the PR.
Sorry about that.
@@ -409,7 +409,7 @@ var Gmail = function(localJQuery) { | |||
|
|||
api.get.storage_info = function() { | |||
var dom = $("[role=contentinfo] a").first(); | |||
var matches = dom.text().trim().match(/(\d+[.,]\d+) GB of (\d+) GB\s+\((\d+)%\)/); | |||
var matches = dom.text().trim().match(/(\d+[.,]\d+) GB of (\d+) GB\s+\((\d+)\)/); |
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.
This regex will still only work for english locales, and wont be applicable to non-english users.
Please see earlier feedback on this point.
This fixes two of the methods
api.helper.get.navigation_count
andapi.get.storage_info
. It returns a fair amount of data back:I'm trying to target the aria labels and accessibility tags, as I think it's a better move to try to parse the page like https://testing-library.com/ library does testing. Looking at the page like a real user or a screen reader would, rather than targeting class names or such, which is a cat-and-mouse game.
I'm also doing console.warns when things aren't found for easier debugging. Throwing an error or console.error seemed a bit too much.
I'm totally open to any advice or feedback!