scarpe-team / scarpe

Scarpe - shoes but running on webview
Other
162 stars 29 forks source link

Fixed the margin shorthand #545

Closed Nj221102 closed 6 months ago

Nj221102 commented 6 months ago

Description

Similar to font shorthand, the margin shorthand was being overwritten by default input of specific margin properties. This PR addresses this issue by implementing the following improvements:

  1. Margin Shorthand Property Fix: The margin shorthand property has been fixed by directly mapping it with specific margin properties in Lacci. This means that the margin shorthand will no longer overwrite the margin property. Instead, when there are four inputs in the margin shorthand, it will be split into four parts and sent to the specific margin properties. When there is only one input, it will write directly to the margin shorthand property.

  2. Consistent Input Pattern: The margin shorthand now adheres to the same input pattern as the original Shoes library, which is [left, top, right, bottom].

  3. Test Coverage: Tests have been added for the margin_parse method, responsible for mapping the margin shorthand to specific properties.

  4. Removal of 'Pacifico' Font: The 'Pacifico' font has been removed from the font_family.rb example. This font was causing path mismatch issues whenever fixtures were generated, leading to failures in GitHub CI tests. Removing it helps avoid confusion for newcomers until the issue is resolved.

  5. Example Illustration: An example has been provided to demonstrate how the margin shorthand now functions.

    
    Shoes.app do
    
    para "All margins with array input", margin:[60,30,80,40]
    
    para "All margins with string input", margin: "30 20 10 20"
    
    para "One Number to set all margins", margin:20
    
    para "Specific property can overwrite shorthand" , margin:[20,10,20,30], margin_top:100
    
    para "margins using a hash input" , margin:{left:20,top:100,bottom:10,right:20}

end


Due to alterations in the parsing method and input pattern for margins, several adjustments were necessary:

1. **Removal of Deprecated Tests**: Tests associated with the previous parsing method, no longer utilized in Calzini, have been eliminated. These tests were causing failures since the method they were designed to assess is no longer in operation.

2. **Alignment of Margin Input Pattern**: The input pattern for margins in examples has been modified to align with the previous fixture. This adjustment was required because the margin shorthand's input format has transitioned from `[left, right, top, bottom]` to `[left, top, right, bottom]`. This change ensures consistency with the pattern employed in the original Shoes library, facilitating seamless integration with existing examples and usage conventions..

### Image(if needed, helps for a faster review)
<img width="475" alt="Screenshot 2024-02-13 at 10 56 58 PM" src="https://github.com/scarpe-team/scarpe/assets/151559388/239886da-d5fc-493f-85d6-4e5ab7464075">

### Checklist

- [ ] Run tests locally
noahgibbs commented 6 months ago

This looks fine. Would you rather remove the one line for props["margin"] in Calzini? I could also just hit merge -- this is certainly an overall improvement.

Nj221102 commented 6 months ago

This looks fine. Would you rather remove the one line for props["margin"] in Calzini? I could also just hit merge -- this is certainly an overall improvement.

I did add that margin in calzini because i was implementing a feature where margin actually writes to margin when there is only one input , instead of writing to four different specific margins, I think you suggested that as well, when we were talking on discord, if you want i can remove it .

Nj221102 commented 6 months ago

sorry i by mistake pressed the close button instead of comment

noahgibbs commented 6 months ago

I did add that margin in calzini because i was implementing a feature where margin actually writes to margin when there is only one input , instead of writing to four different specific margins, I think you suggested that as well, when we were talking on discord, if you want i can remove it .

I think we definitely do not want to write to the Shoes style margin in that case. But we may want to set the CSS property, as a detail of the Webview implementation. Make sense?

Nj221102 commented 6 months ago

I did add that margin in calzini because i was implementing a feature where margin actually writes to margin when there is only one input , instead of writing to four different specific margins, I think you suggested that as well, when we were talking on discord, if you want i can remove it .

I think we definitely do not want to write to the Shoes style margin in that case. But we may want to set the CSS property, as a detail of the Webview implementation. Make sense?

got that , I am removing the margin , making the commit in 10 mins 👍

Nj221102 commented 6 months ago

@noahgibbs done 👍