Skip to content

Add accessibility id to navigation title#1127

Merged
tomholub merged 9 commits intomasterfrom
feature/issue-1094-accessibility-id-title
Dec 6, 2021
Merged

Add accessibility id to navigation title#1127
tomholub merged 9 commits intomasterfrom
feature/issue-1094-accessibility-id-title

Conversation

@Kharchevskyi
Copy link
Contributor

@Kharchevskyi Kharchevskyi commented Nov 30, 2021

This PR adds accessibility id to navigation title

close #1094


Tests:

  • Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@Kharchevskyi
Copy link
Contributor Author

@fcvakintos can you please take a look if this solution works for you?

@fcvakintos
Copy link
Contributor

@Kharchevskyi it doesn't work for me.
no accessibility Id for headers
Screenshot 2021-12-01 at 10 07 37

@Kharchevskyi
Copy link
Contributor Author

Got it. Will then try another solution.
Will keep you in touch

@Kharchevskyi
Copy link
Contributor Author

Screenshot 2021-12-01 at 10 49 46

@fcvakintos Please check if this what you are looking for. If yes, I will update PR with normal UI

@fcvakintos
Copy link
Contributor

@Kharchevskyi doesnt work for me
Screenshot 2021-12-01 at 11 07 05

@Kharchevskyi
Copy link
Contributor Author

@fcvakintos please check latest commit.

@fcvakintos
Copy link
Contributor

@Kharchevskyi doesnt work

@Kharchevskyi
Copy link
Contributor Author

let's schedule a meeting and try to find where the problem is
https://github.com/FlowCrypt/org - you can fin email ther

@tomholub
Copy link
Collaborator

tomholub commented Dec 1, 2021

@fcvakintos could you also please demonstrate how can we inspect the layout the way you are inspecting it?

@fcvakintos
Copy link
Contributor

@Kharchevskyi
Copy link
Contributor Author

I can try to setup it and try locally, but I would need some time, because I'm not familiar with Appium

@tomholub
Copy link
Collaborator

tomholub commented Dec 1, 2021

Please try, and document the steps so that another person not familiar with appium can do the same. Ask @fcvakintos if you get stuck.

That way we'll all be able to debug these things directly.

@Kharchevskyi
Copy link
Contributor Author

@fcvakintos can you please share your config for https://github.com/appium/appium-inspector
Screenshot 2021-12-03 at 21 59 25

@fcvakintos
Copy link
Contributor

@Kharchevskyi please replace path to app

{
"platformName": "iOS",
"iosInstallPause": 5000,
"deviceName": "iPhone 13",
"app": "path to app",
"platformVersion": "15",
"automationName": "XCUITest",
"newCommandTimeout": 10000,
"wdaLaunchTimeout": 300000,
"wdaConnectionTimeout": 600000,
"wdaStartupRetries": 4,
"wdaStartupRetryInterval": 120000
}

@tomholub
Copy link
Collaborator

tomholub commented Dec 3, 2021

The inspector should be added to package.json so it gets auto-installed. That means fewer steps for someone who wants to use it.

@fcvakintos
Copy link
Contributor

we don't need it in package.json

@tomholub
Copy link
Collaborator

tomholub commented Dec 3, 2021

I didn't say we need it. I said that if we add it there, then other developers who need to use it don't have to install it separately. Isn't that true?

@fcvakintos
Copy link
Contributor

I am using mac app installation, not npm

@tomholub
Copy link
Collaborator

tomholub commented Dec 3, 2021

Understood - I assumed it could be installed from npm, which is not the case.

@Kharchevskyi
Copy link
Contributor Author

Kharchevskyi commented Dec 4, 2021

@fcvakintos would this work for you?
Screenshot 2021-12-04 at 18 26 40

@fcvakintos
Copy link
Contributor

@Kharchevskyi you don't have accessibility Id for header now
check screen, here is accessibility id

Screenshot 2021-12-06 at 10 22 51

@Kharchevskyi
Copy link
Contributor Author

@fcvakintos please confirm this is what you are looking for
Screenshot 2021-12-06 at 13 36 01

@fcvakintos
Copy link
Contributor

@Kharchevskyi we can use "choosePassPhrase" - it is better i think

@Kharchevskyi
Copy link
Contributor Author

according to suggestion
#1127 (comment)

@fcvakintos
Copy link
Contributor

@Kharchevskyi yes, but it is not correct, i think it should be 1 word, can we change it?

@tomholub
Copy link
Collaborator

tomholub commented Dec 6, 2021

@fcvakintos if it should be one word, then I suggest navigationItemChoosePassPhrase which would be done dynamically something like navigationItem\(title.replace(" ", "")). @Kharchevskyi could you do that?

@tomholub
Copy link
Collaborator

tomholub commented Dec 6, 2021

@Kharchevskyi later please add to appium/README.md how to install and use the above tool and how to configure it so that others can also do it. I'm filing a separate issue.

@Kharchevskyi
Copy link
Contributor Author

Yes, sure. Will make it as one word - navigationItemChoosePassPhrase and so on

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

I assume this is ready to merge

@tomholub tomholub marked this pull request as ready for review December 6, 2021 16:26
@tomholub
Copy link
Collaborator

tomholub commented Dec 6, 2021

@Kharchevskyi or not yet?

@tomholub
Copy link
Collaborator

tomholub commented Dec 6, 2021

I'll merge it either way, if there is work left to do, can do another PR.

@tomholub tomholub merged commit 50dbc9f into master Dec 6, 2021
@tomholub tomholub deleted the feature/issue-1094-accessibility-id-title branch December 6, 2021 16:27
@fcvakintos
Copy link
Contributor

@Kharchevskyi @tomholub I found new issue after this fix, now we dont have accesibility id for menu bar items
it is after this fix i think, because now accessibility id = "INBOX" is not unique
Screenshot 2021-12-07 at 10 26 21

can we add accessibility id for menu bar items?
something like this - menubarIteminbox, menubaritemchat and etc

@Kharchevskyi
Copy link
Contributor Author

Can't be related to this changes, but will do asap

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.

Add accessibility ID for headers

4 participants