[SPT-1480] RDDM Accessibility optimizations by Ikeret · Pull Request #239 · surfstudio/ReactiveDataDisplayManager

Ikeret · 2023-06-22T12:46:18Z

Что сделано?

Переделаны стратегии

Зачем это сделано?

Новая организация стратегий позволит более удобно их использовать и открывает больший функционал для расширения

На что обратить внимание?

Еще в планах оптимизировать работу с UIAccessibilityCustomActions
Если изменения ок, то в конце будут исправлены тесты и Example

Пример:

Если раньше стратегии задавались очень длинно и нагроможденно

 var labelStrategy = .joined([.from(object: titleLalel), .just(", "), .from(object: offerLabel)])

var valueStrategy = .joined([
    .just("Количество: \(stepper.value), по цене "),
    .from(object: priceLabel),
    .just(oldPriceLabel.text.map { ". Старая цена \($0)" })
])

то теперь станет это проще

var labelStrategy = .merge(titleLabel.text, offerLabel.text, separator: ", ")

var valueStrategy = .merge(
    "Количество: \(stepper.value), по цене ", 
    priceLabel.text, 
    oldPriceLabel.text.map { ". Старая цена \($0)" }
)

github-actions · 2023-06-22T12:47:16Z

	Warnings
⚠️	Oops! We have found some issues. It's better to fix them to keep code clean

SwiftLint found issues

Severity	File	Reason
Warning	AccessibilityItemWrapper.swift:18	Redundant explicit declaration of default types should be avoided (`redundant_default_type`)
Warning	AccessibilityItem+Invalidation.swift:90	Line should be 145 characters or less: currently 158 characters (`line_length`)

Generated by 🚫 Danger Swift against 72f69d7

NullIsOne

преаппрув
хороший генерик

NullIsOne · 2023-06-22T13:31:29Z

Source/Protocols/Plugins/Generators/Accessibility/AccessibilityItem+Strategies.swift

-        }
+    /// a reference value from object.
+    /// - parameter keyPath: specified keypath from value would be taken.
+    public static func from<T>(_ object: T, keyPath: KeyPath<T, ValueType?>) -> Self {


Вот тут хорошо что от привязки к NSObject ушли.
Это ограничивало варианты источников

Да, теперь работает с любым объектом, можно передавать свои классы и указывать их поля)

Ikeret · 2023-07-04T16:35:38Z

Добавлен контейнер для итемов, который позволит задавать в ячейке чайлд accessibility итемы
Добавлен враппер для итема, позволит оборачивать в AccessibilityItem вьюшки, к которым нет доступа (к примеру вью из библиотек)
Немного обновлены стратегии
Исправлен Example

Осталось:

придумать удобный способ задания UIAccessibilityCustomActions
подумать над частичной инвалидацией параметров
подумать как использовать логику для обычных вью (сейчас можно использовать вью итемы в контейнере, но логика изменения в плагине остается)
возможно стоит вынести всю Accessibility логику в отдельный таргет/либу (оторвать от RDDM 😢)
добавить/обновить документацию по новой логике

codecov-commenter · 2023-07-04T16:39:33Z

Codecov Report

Patch coverage: 58.83% and project coverage change: +26.70 🎉

Comparison is base (d604813) 36.56% compared to head (eca573b) 63.26%.

❗ Current head eca573b differs from pull request most recent head 0a4f73b. Consider uploading reports for the commit 0a4f73b to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##           release/7.4     #239       +/-   ##
================================================
+ Coverage        36.56%   63.26%   +26.70%     
================================================
  Files              143      151        +8     
  Lines             5303     5491      +188     
  Branches          2437     2497       +60     
================================================
+ Hits              1939     3474     +1535     
+ Misses            3283     1886     -1397     
- Partials            81      131       +50

Flag	Coverage Δ
uitests	`48.11% <51.97%> (?)`
unittests	`35.36% <18.31%> (-1.20%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...llection/DataSource/BaseCollectionDataSource.swift	`77.41% <0.00%> (+38.70%)`	⬆️
...Collection/Modifier/CollectionCommonModifier.swift	`46.03% <0.00%> (+3.43%)`	⬆️
...llection/Modifier/CollectionDiffableModifier.swift	`51.47% <0.00%> (+51.47%)`	⬆️
.../Collection/Protocols/CollectionFoldableItem.swift	`0.00% <0.00%> (ø)`
Source/Protocols/Modifier/Modifier.swift	`10.00% <0.00%> (-1.12%)`	⬇️
...cessibility/Container/AccessibilityContainer.swift	`0.00% <0.00%> (ø)`
...ssibility/Container/AccessibilityItemWrapper.swift	`0.00% <0.00%> (ø)`
...ators/Accessibility/Item/AccessibilityAction.swift	`0.00% <0.00%> (ø)`
...lity/Modifier/AccessibilityContainerModifier.swift	`0.00% <0.00%> (ø)`
...ce/Protocols/Plugins/Generators/FoldableItem.swift	`0.00% <0.00%> (ø)`
... and 42 more

... and 49 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

NullIsOne · 2023-07-13T08:10:48Z

Апдейт

🛠️ придумать удобный способ задания UIAccessibilityCustomActions
🛠️ подумать над частичной инвалидацией параметров
✅ подумать как использовать логику для обычных вью (сейчас можно использовать вью итемы в контейнере, но логика изменения в плагине остается)
Достаточно вызвать modify и при необходимости настроить инвалидатор. (проверено на Playbook)
Опишу способы использования в доке.
❌ возможно стоит вынести всю Accessibility логику в отдельный таргет/либу (оторвать от RDDM 😢)
В 7.4 зарелизим в RDDM. В 8.0 вынесем как podspec и package product для SPM.
Такой подход необходим для уменьшения издержек от поддержки 7.3.x/7.4 и develop.
🛠️ добавить/обновить документацию по новой логике

NullIsOne · 2023-07-17T13:04:00Z

Апдейт

✅ придумать удобный способ задания UIAccessibilityCustomActions
Создана AccessibilityActionsStrategy и структура враппер AccessibilityAction
Применение в ПР appTemplate
✅ 50/50 подумать над частичной инвалидацией параметров
Добавлен базовый инвалидатор, который не лезет за генератором. Полезно в playbook например.
Инвалидацию label, value, traits, actions отдельно друг от друга мешает сделать AccessibilityModifier который берет все из AccessibilityItem
Оптимизируем этот момент в 8.0

Осталось
🛠️ добавить/обновить документацию по новой логике

NullIsOne · 2023-07-18T09:00:51Z

Переделал включенный по-умолчанию acessibilityPlugin
Поправил документацию
Готово к review

Ikeret

Есть небольшой вопрос по ContainerModifier, ну а так все круто)

Ikeret · 2023-07-18T16:45:03Z

Source/Protocols/Plugins/Generators/Accessibility/Container/AccessibilityItemWrapper.swift

+    open var modifierType: AccessibilityModifierType = AccessibilityItemModifier.self
+
+    required public init(
+        parentView: UIView,


тут required будет мешать, надо отдельный инит required public init(parentView: UIView) добавить, а стратегии и так open

required нужен, чтобы обязательно инициализировать через super.init(accessibilityContainer: parentView) - без него итем не будет корректно отображаться (опыт с прошлого проекта)

имею в виду один инит required public init(parentView: UIView), а второй как сейчас, но не required

добавил not required init

Ikeret · 2023-07-18T16:47:13Z

Source/Protocols/Plugins/Generators/Accessibility/Item/AccessibilityItem+Providers.swift

 public extension AccessibilityStrategyProvider {
    var valueStrategy: AccessibilityStringStrategy { .ignored }
+    var traitsStrategy: AccessibilityTraitsStrategy { .ignored }
+    var actionsStrategy: AccessibilityActionsStrategy { .ignored }


Это хорошо, .ignored не хватало, позволят использовать стандартное accessibilityActions при необходимости

Ikeret · 2023-07-18T16:51:48Z

Source/Protocols/Plugins/Generators/Accessibility/Modifier/AccessibilityContainerModifier.swift

+            return
+        }
+
+        item.isAccessibilityElement = !item.isAccessibilityIgnored


Не совсем уверен нужно ли тут убирать isAccessibilityElement

Нашему контейнеру не нужно быть AccessibilityElement.
Если помнишь по опыту TDW AccessibilityElement, который содержит AccessibilityElement перекрывает child элементы.
При отладке на проекте это подтвердилось и с AccessibilityContainer.

kombatkos · 2023-07-19T11:46:13Z

...playManager/Collection/AlignedCollectionViewController/AlignedCollectionViewController.swift

    // MARK: - Private Properties

    private lazy var adapter = collectionView.rddm.baseBuilder
+        .add(plugin: .accessibility())


Этот контроллер не рабочий

В каком плане?
По a11y там минимальная адаптация с указанием "some image".
Там layout который к левой или к правой стороне прижимает. И больше никакой логики.

Или в прошлых версиях iOS другой эффект был?

Ikeret requested review from Golubeykov and NullIsOne June 22, 2023 12:46

Ikeret self-assigned this Jun 22, 2023

Ikeret marked this pull request as draft June 22, 2023 12:54

NullIsOne reviewed Jun 22, 2023

View reviewed changes

NullIsOne added 7.4 enhancement labels Jun 22, 2023

Golubeykov approved these changes Jun 23, 2023

View reviewed changes

NullIsOne force-pushed the release/7.4 branch from cf14ca4 to a595253 Compare July 12, 2023 12:32

NullIsOne force-pushed the feature/SPT-1480-optimizations branch 2 times, most recently from f40ba86 to 02f5b2c Compare July 12, 2023 13:49

NullIsOne marked this pull request as ready for review July 18, 2023 09:00

NullIsOne self-assigned this Jul 18, 2023

NullIsOne requested a review from kombatkos July 18, 2023 09:08

Ikeret commented Jul 18, 2023

View reviewed changes

kombatkos reviewed Jul 19, 2023

View reviewed changes

kombatkos approved these changes Jul 19, 2023

View reviewed changes

NullIsOne force-pushed the release/7.4 branch from a595253 to f5b9339 Compare July 19, 2023 12:01

NullIsOne and others added 6 commits July 19, 2023 16:03

fix Unit and UI tests

10b5135

added traits provider for base protocols

84e7b13

added table views accessibility

8786d07

add footers for table

197abc3

make title collection view cell calcualtable (issues from 44 to 40)

1dc8f76

refactor strategies

e17ae61

Ikeret and others added 14 commits July 19, 2023 16:03

moved files into folders & added AccessibilityContainer

8e9482c

extend strategy methods

876911f

fixed example

5b33586

add shortcuts to improve modification out of rddm

d06e8ad

make container not acessible element if accessibility is ignored

caf458b

fix rebase conflicts

dfbaa61

update AccessibilityItemInvalidator protocol and implementations

bb209f4

add filtration for a11y string values

8a2cdef

add AccessibilityAction and Strategy to apply it in Modifier

795c1bd

fix linter issues

2bdde92

make invalidator configurable

58f59fa

fix isAccessibilityIgnored

d25de15

update documentation

b5bb525

fix init for ItemWrapper

72f69d7

NullIsOne force-pushed the feature/SPT-1480-optimizations branch from 0a4f73b to 72f69d7 Compare July 19, 2023 12:03

NullIsOne merged commit 5462585 into release/7.4 Jul 21, 2023

NullIsOne deleted the feature/SPT-1480-optimizations branch July 21, 2023 11:22

Conversation

Ikeret commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Что сделано?

Зачем это сделано?

На что обратить внимание?

Пример:

Uh oh!

github-actions bot commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SwiftLint found issues

Uh oh!

NullIsOne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ikeret commented Jul 4, 2023

Uh oh!

codecov-commenter commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

NullIsOne commented Jul 13, 2023

Uh oh!

NullIsOne commented Jul 17, 2023

Uh oh!

NullIsOne commented Jul 18, 2023

Uh oh!

Ikeret left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ikeret commented Jun 22, 2023 •

edited

Loading

github-actions bot commented Jun 22, 2023 •

edited

Loading

codecov-commenter commented Jul 4, 2023 •

edited

Loading

Ikeret left a comment •

edited

Loading