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

[iOS] Implement VerticalTextAlignment property in Label #3714

Merged
merged 22 commits into from
Mar 25, 2022

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

Implement VerticalTextAlignment property in iOS Label.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

Does this PR touch anything that might affect accessibility?

  • No

@jsuarezruiz jsuarezruiz added platform/iOS 🍎 legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor labels Dec 9, 2021
@jsuarezruiz
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jsuarezruiz jsuarezruiz marked this pull request as ready for review December 13, 2021 11:45
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

   D:\agent\1\s\src\Core\src\Platform\iOS\MauiLabel.cs(37,64): error CS0246: The type or namespace name 'nfloat' could not be found (are you missing a using directive or an assembly reference?) [D:\agent\1\s\src\Core\src\Core-net6.csproj]

We hve to move to new code of nfloat

@jsuarezruiz
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@Redth Redth left a comment

Choose a reason for hiding this comment

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

@jsuarezruiz i'm seeing a crash on catalyst / ios can you please investigate?

@Eilon Eilon added the area-controls-label Label, Span label Feb 11, 2022
@jsuarezruiz
Copy link
Contributor Author

Updated fixing the issue and checked that is working as expected:
Captura de pantalla 2022-02-15 a las 12 57 09

Copy link
Member

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

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

I see VerticalTextAlignment.End (blue) is working, but it doesn't seem like VerticalTextAlignment.Start (pink) and VerticalTextalignment.Center (yellow) are

                <Label 
                    BackgroundColor="Pink"
                    VerticalTextAlignment="Start"
                    Text="This should be at the top"
                    HeightRequest="100"/>
                <Label 
                    BackgroundColor="Yellow"
                    VerticalTextAlignment="Center"
                    Text="This should be at the middle"
                    HeightRequest="100"/>
                <Label 
                    BackgroundColor="Blue"
                    VerticalTextAlignment="End"
                    Text="This should be at the bottom"
                    HeightRequest="100"/>

Simulator Screen Shot - iPad mini (6th generation) - 2022-02-15 at 11 36 21

@jsuarezruiz
Copy link
Contributor Author

Updated:
ios-verticaltextalignment

@Redth Redth self-requested a review March 15, 2022 18:10
@Redth Redth self-assigned this Mar 15, 2022
@Redth Redth added this to the 6.0.300-rc.1 milestone Mar 15, 2022
@Redth Redth assigned rmarinho and unassigned jsuarezruiz, rachelkang and Redth Mar 15, 2022
@Redth
Copy link
Member

Redth commented Mar 23, 2022

Still freezing on Catalyst... not sure why...

@PureWeen PureWeen assigned PureWeen and unassigned rmarinho Mar 24, 2022
@PureWeen
Copy link
Member

PureWeen commented Mar 24, 2022

I fixed the infinite loop and VTA if you're not using a background on the label.

When you use a background on a Label it places the UILabel inside a WrapperView. The WrapperView resets the Frame every time a layout occurs.

I didn't find an obvious way to account for this so I think we should just merge this and log a bug for the WrapperView variation.

The original PR had kind of the opposite problem. It worked in some cases with the WrapperView because that's how the original code from XF worked where everything was wrapped. I don't think setting your own Frame inside LayoutSubviews is a good way to go. In Forms this was fine because the UILabel was a SubView of the VisualElementRenderer so the order of things made sense.

I changed the code so it sets the Frame from Handler.Arrange which is going to occur right before LayoutSubviews. The previous solution probably worked for wrapped views because the Frame was being updated inside LayoutSubviews but I think it would be better to get everyone's size figured out before the platform reaches LayoutSubviews. We probably want to add some code so the WrappedView can call Arrange or something on its child instead of just replacing the `Frame``

image

Copy link
Member

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

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

Doesn't appear to be working for me on catalyst :/

Screenshot 2022-03-25 at 10 00 47 AM

@PureWeen
Copy link
Member

Doesn't appear to be working for me on catalyst :/

#3714 (comment)

#5575

@rachelkang
Copy link
Member

Doesn't appear to be working for me on catalyst :/

#3714 (comment)

#5575

confirming it works without the background color!

@rmarinho rmarinho disabled auto-merge March 25, 2022 16:39
@rmarinho rmarinho merged commit b87d38a into main Mar 25, 2022
@rmarinho rmarinho deleted the ios-label-verticaltextalignment-2 branch March 25, 2022 16:40
@github-actions github-actions bot locked and limited conversation to collaborators Dec 24, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
@samhouts samhouts added the fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-label Label, Span fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! platform/iOS 🍎
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants