Skip to content
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

[Horizon] Icon Buttons #3036

Open
wants to merge 11 commits into
base: feature/horizon
Choose a base branch
from

Conversation

reabbotted
Copy link

@reabbotted reabbotted commented Dec 17, 2024

No description provided.

@inst-danger
Copy link
Contributor

inst-danger commented Dec 17, 2024

Horizon Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Dec 18, 2024

Fails
🚫

Please add a release note. If no release note is wanted, use none. Example: release note: Fixed a bug that prevented users from enjoying the app.

🚫

Please add which apps this change affects. Example: affects: Teacher, Student or affects: none

🚫

Please add a reference to a Jira ticket. For example: refs: MBL-10023

🚫 Build failed, skipping coverage check
❌ XCTest failed: CoreTests/AssignmentCellViewModelTests/testSubmissionStatusAndIconAndColor
XCTAssertEqual failed: ("#6a7883") is not equal to ("#697783")
XCTAssertEqual failed: ("#03893d") is not equal to ("#03893c")

Generated by 🚫 dangerJS against 80af03c

Updating the access levels for some of the items so they can be used in the project
Copy link
Contributor

@Ahmed-Naguib93 Ahmed-Naguib93 left a comment

Choose a reason for hiding this comment

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

Nice work, but i set some comments

Copy link
Contributor

@Ahmed-Naguib93 Ahmed-Naguib93 left a comment

Choose a reason for hiding this comment

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

Thanks for your work, is working fine for me, but i have one question and one suggestion
1- Question: In the case where the icon button is disabled but has a badge, should we set the opacity for the badge view or hide it? I couldn’t find this scenario in Figma. We can ask the team about it after your holiday.

2- Suggestion: What is your opinion on using a scenario for showing the icon button that makes it more readable and usable?

public extension HorizonUI {
    struct IconButton: View {
        let type: HorizonUI.ButtonStyles.ButtonType
        let isSmall: Bool
        let badgeNumber: String?
        let icon: Image?
        let onTap: () -> Void

        public init(
            type: HorizonUI.ButtonStyles.ButtonType,
            isSmall: Bool = false,
            badgeNumber: String? = nil,
            icon: Image? = nil,
            onTap: @escaping () -> Void
        ) {
            self.type = type
            self.isSmall = isSmall
            self.badgeNumber = badgeNumber
            self.icon = icon
            self.onTap = onTap
        }

        public var body: some View {
            Button("") {
                onTap()
            }
            .buttonStyle(
                HorizonUI.ButtonStyles.icon(type, isSmall: isSmall, badgeNumber: badgeNumber, icon: icon)
            )
        }
    }
}

public func makeBody(configuration: Configuration) -> some View {
ZStack {
if let icon = icon {
ZStack {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make if let icon this is enough & create two functions for each branch as you did before, it was more readable

if let icon {
iconButton()
} else {
normalButton()
}

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.

3 participants