-
Notifications
You must be signed in to change notification settings - Fork 724
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
Interface Builder: accessibility identifiers #369
base: stable
Are you sure you want to change the base?
Conversation
8ad6976
to
f664e98
Compare
0ad06cc
to
9446b97
Compare
Thanks for the PR! We'll try to finish the merge back into monorepo by cleaning a few stuff but will soon look at it right after 👍 Concerning the new command —
I've open the debate in a dedicated Team Discussion on GitHub: https://github.com/orgs/SwiftGen/teams/corecontributors/discussions/3 |
Yeah that's a discussion we've had a few times here on GitHub, and in slack. I'll see if I can dig up some of the points that were made then. |
Another open discussion is: shouldn't those For now, the templates seem to only generate constants convertible to strings, which means that nothing would prevent you to do That could indeed be a first take / first version to at least start introducing the feature, that we could then improve later by making it more type-safe, but I was wondering if we couldn't imagine a solution more akin to what we do with StoryboardScenes, where each constant is typed (using generics) to tell which type of But to make it more strongly-typed, maybe instead we can go the full type-safe route, like this? struct A11yElement {
var type: XCUIElement.ElementType
var id: String
}
extension XCUIApplication {
subscript(id: A11yElement) -> XCUIElement {
return self.elementMatching(type: id.type, identifier: id.id)
}
}
// Generated code:
enum A11y {
enum ChildViewController
static let myButton = A11yElement(type: .button, id: "MyButton")
static let myTextField = A11yElement(type: .textfield, id: "MyTextField")
}
…
} So we can then use it like this: let b = app[A11y.ChildViewController.myButton]
let f = app[A11y.ChildViewController.myTextField] I'm sure if we brainstorm further, we can even imagine something even more useful when you execute some UI test on a single VC and want to access a lot of identifiers attached to the same VC, something that would allow us to do things like this? let avc = A11y.ChildViewController(app)
let b = avc(.myButton)
let f = avc(.myTextField) But maybe that last idea is too complex for a first take… |
I'm happy to integrate this as part of the existing commands, im worried however that since this command deals with storyboards and xibs that it may be confusing if it also deals with only storyboard based data (segues etcs) ? As for the typing, generally all you need is the string for a lookup. There are some lookup functions in which you can narrow the search using an element type also - but I am unsure on how problematic it may be to extract those also? Funny about your last point, the reason I built this is to facilitate a new UI testing feature im building for work which uses the output from this to achieve type safe/compiler checking of UI tests 😄 i.e my framework gives you:
and |
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.
Hey 😉
Even if the debate about whether to piggy-back on the existing subcommands or create a dedicated one is still open:
-
You should rename your templates to include the Swift version they are for (swift3 and/or swift4). It would even be nice to have one version of each template for each swift version, so
enums-swift3
,enums-swift4
,properties-swift3
,properties-swift4
-
Maybe even if for now we keep the dedicated subcommand, maybe because the output stencil context has a hierarchy that is a bit too different from what's generated by
storyboards
, I wonder if the future feature supportingUITableViewCell
/UICollectionViewCell
'sreuseIdentifiers
would maybe end up with a very similar structure / hierarchy in the context, and thus will share the same command? In that spirit, naming itidentifiers
might seem more adequate? Or if not and we consider still keeping a dedicated command, we might want to renaming it to something shorter, likeswiftgen a11y
? -
In your tests, all accessibility identifiers you use in the different storyboards & XIB happen to have always the same name (
button
ortextfield
). So that if by mistake your implementation happen to mess up the various identifiers or merge them between storyboards by mistake, we won't catch this kind of bug. Maybe it would be nice to keep one or two common (to still test the case that we properly avoid collisions) but also have others that are different (to also test we don't mix them up)?
Funny that you mention that, as I originally replied to your previous comment by saying that we could make the constants not But then as I was writing that I realised that it was not safe enough (in the sense that, if we're gonna generate type-safe constants, the goal is to make it way safer than using just a string, so as we're at it better make it extra-safe even), because one might have multiple elements with the same |
@AliSoftware thanks for spending some time on this 🤘
I'm not sure then how you'd like me to proceed re: |
About |
Mmmmh but now that I think about it, if the template use the accessibilityIdentifier's String to determine the name of the constant to generate… we might still have conflicts indeed… like: // Static code in the generated output, not depending on the files parsed
struct A11yElement {
let name: String
let type: XCUIElement.ElementType
private init(name: String, type: XCUIElement.ElementType) {
self.name = name
self.type = type
}
}
// Code generated according to files parsed
enum A11y {
enum SomeViewController {
static let foo = A11yElement(name: "foo", type: .button)
static let foo = A11yElement(name: "foo", type: .textfield)
}
} So indeed in that case as the generated constant's name is likely to be determined by the Still, if it's not too complicated, at least maybe it's still worth it including the |
Yeah I'll look into adding the element type, finding them isn't hard - it seems the Also re: uniqueness - I think its a happy side effect that collision would fail.. this will enforce uniqueness which you could argue is just another reason SwiftGen adds safety (imo this is better than potential collisions) |
also, you're thoughts on my q1 above ? |
Yeah, I happen to agree (it's an identifier after all, they generally are meant to be unique)… … but we also have to remember that to each their own and that some other people might have different habits and PoVs (we had that experience/feedback in the past especially with the way some people organise After all the whole power of SwiftGen is its template system and ability for people to customize the output according to how they work, so it's always good to consider that some users might not have the same rules (eg about uniqueness) as we both do 😉 |
@AliSoftware so im getting close to having the element type as part of the output however another potential issue is going to be the depending on the use case that'll mean more templates, do you have any thoughts on this ?? |
I'm not sure I get how that would be an issue… I mean:
So yeah, that would mean more templates (one template only compatible with test targets — that I'm not sure we're gonna provide just yet, but might exist in the future or in custom templates — and one not using |
ok sounds good, in that case I won't worry about creating the |
I still don't understand why you'd need So in the parser you'll just need to create a Then when you parse your XIBs/Storyboards and generate the Stencil context entries for each accessibility identifier, you can build that entry using the accessibilityID and the string corresponding to the elementType (so that people will be able to use it in their own templates and generate text which happen to be valid Swift referencing the element type) Sure, if we could import |
9446b97
to
83e3185
Compare
@AliSoftware Finally got a chance to get back to this.., I've:
Lemme know what you think 🤘 |
83e3185
to
d91cedb
Compare
d91cedb
to
4040c2d
Compare
|
||
{{accessModifier}} enum AccessibilityIdentifier { | ||
{% for container in containers %} | ||
{{accessModifier}} enum {{container.name}}: String { |
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.
The fact that enum
here is declaring String
rawValue is causing a compiler error, where it says enum with no cases shouldn't declare a rawValue.
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.
@Mazyod @IanKeen Would the compiler error go away if we add a conditional statement that checks the size of the array before actually iterating through them?
{{accessModifier}} enum AccessibilityIdentifier {
{% for container in containers %}
{% if container.accessibility.count > 0 %}
{{accessModifier}} enum {{container.name}}: String {
{% for accessibility in container.accessibility %}
{{accessModifier}} static let {{accessibility.identifier}} = "{{accessibility.rawValue}}"
{% endfor %}
}
{% endif %}
{% endfor %}
}
}
I imagine this would avoid an instance where you get an enum that has no cases.
2021, what's the status of this? 😬 |
Oh wow this would be great to have working indeed :) |
@acosmicflamingo I apologize if that's what it sounded like. You are right |
Oh I wasn't accusing you of sounding like that; I was totally talking about general cases! I'm sorry for making you think that 🙂 |
Maybe 2024 will be the year :) |
This PR adds a new type of output to SwiftGen, it will parse storyboards and xibs looking for accessibility identifiers.
I have included two templates,
enums
:and
properties
: