-
Notifications
You must be signed in to change notification settings - Fork 1
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
[#405] Add switch component #439
base: develop
Are you sure you want to change the base?
Conversation
759bc02
to
0c0542a
Compare
UI tests are missing |
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.
Pas mal de typos à corriger.
Un peu de réorganisation de code.
Un peu de renaming.
Le fichier NOTICE pas à jour.
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Added | |||
|
|||
- [DesignToolbox] Add text field in component configuration to customize text ([#436](https://github.com/Orange-OpenSource/ouds-ios/issues/436)) | |||
- [Library] Switch component ([#405](https://github.com/Orange-OpenSource/ouds-ios/issues/405)) |
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.
Je mettrais cette ligne en haut, avant celle relative à la carte n°436
@@ -197,7 +197,7 @@ struct ButtonConfiguration: View { | |||
} | |||
|
|||
if model.layout == .iconAndText || model.layout == .textOnly { | |||
DesignToolboxTextField(text: $model.text, prompt: "app_component_common_userText_prompt") | |||
DesignToolboxTextField(text: $model.text, prompt: "app_components_common_userText_prompt") |
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.
Je trouve curieux d'utiliser du camel case en plus du snake case, c'est normal d'avoir "userText" et pas par exemple "user_text" ? Je ne connais pas les règles appliquées au wording ici :)
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.
cf. règles de naming dans le fichier
@@ -26,9 +26,9 @@ struct EmptyState: View { | |||
.frame(width: 160, height: 160, alignment: .center) | |||
|
|||
VStack(alignment: .center, spacing: theme.spaces.spaceFixedShorter) { | |||
Text("app_component_emptyContent_text") | |||
Text("app_components_emptyContent_text") |
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.
Idem que précédemment, "empty_content" plutôt ?
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.
cf règles de naming
@@ -167,7 +167,7 @@ struct LinkConfiguration: View { | |||
} | |||
} | |||
|
|||
DesignToolboxTextField(text: $model.text, prompt: "app_component_common_userText_prompt") | |||
DesignToolboxTextField(text: $model.text, prompt: "app_components_common_userText_prompt") |
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.
"user_text" plutôt que "userText" ?
|
||
// MARK: - Switch Configuration Model | ||
|
||
/// The model shared between `SwitchPageConfiguration` view and `SwitchPageComponent` view. |
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.
SwitchPageConfiguration n'existe pas, SwitchPage plutôt ?
De même, SwitchPageComponent n'existe pas.
|
||
// MARK: - Stored properties | ||
|
||
@Environment(\.theme) private var theme |
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.
Pmutôt :
let show: Bool
@Environment(\.theme) private var theme
@Environment(\.colorScheme) private var colorScheme
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.
on mis les env en début partout dans le code
import SwiftUI | ||
|
||
/// A `ViewModifier` which will apply a specific divider under a `View` using color semantic token. | ||
public struct DividerModifier: ViewModifier { |
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.
Attention, c'est une API publique, donc il faut indiquer dans la doc le "Since"
/// ```swift | ||
/// var body: some View { | ||
/// SomeView() | ||
/// .divider() |
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.
Le code sample n'est ps cohérent ; tu utilsies "divider" et non "oudsDivider(show:)", c'est normal ?
|
||
extension View { | ||
|
||
/// Modifies the current `View` to apply a divider under. |
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.
"below" plus que "under"
/// SomeView() | ||
/// .divider() | ||
/// ``` | ||
/// - Parameter: |
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.
Si tu builds al doc en local tu verras un warning sur ça.
Il faut que ce soit "Parameter show:" plutôt que de faire un saut de ligne (ça amrche avec "Parameters"
Note: Please transform
- [ ]
into- (NA)
in the description when things are not applicableRelated issues
#405
Description
Motivation & Context
Implement component of design system
Types of change
Previews
Checklist
Contribution
Accessibility
Design
Development
Documentation
Checklist (for Core Team only)