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

Fixed the font property #541

Merged
merged 5 commits into from
Feb 8, 2024
Merged

Fixed the font property #541

merged 5 commits into from
Feb 8, 2024

Conversation

Nj221102
Copy link
Contributor

@Nj221102 Nj221102 commented Feb 6, 2024

Description

previously font- shorthand property wasn't working properly because the specific property for the font size was overwriting the font property and it has a default value so it was never nil, hence making the size section in the font useless.

so to overcome this problem and fix it, I made some changes so now font wouldn't write to a font property, but instead to the specific ones ,which will do three things:-

  1. if there is no font property then specific properties will work as it is.
  2. but if there are some properties mentioned in the shorthand property then those properties will overwrite their respective specific property, which means the things written in the font will applied,not what's written in specific ones.
  3. if there is some property which is not present in font property, like family, italic, or weight is present in font-shorthand but no size input is there, then in this case if you write the size in specific :size property ,then it will also work.

All these features are similar to how shoes used to work

Shoes.app do
    
    para "The family is not in font so it will change but size won't" , font:"italic bold 30px" , family:"cursive", size: 20

    para "because  only things things who aren't present in font " , font:"bold", size: "16px"

    para "will be used as their specific property", font: "cursive italic 20px ", font_weight:"bold"

end

Image(if needed, helps for a faster review)

Screenshot 2024-02-07 at 1 42 19 AM

Checklist

  • Run tests locally

@noahgibbs
Copy link
Collaborator

I'm also seeing some odd things with quoted font family versus unquoted (e.g. font: "Pacifico" vs font: "'Pacifico'"). Probably time to add some tests...

@Nj221102
Copy link
Contributor Author

Nj221102 commented Feb 8, 2024

@noahgibbs I have added the test for the parsing logic
Screenshot 2024-02-08 at 12 40 07 PM

Is there a need for further changes, please suggest .

@@ -1,7 +1,8 @@
# frozen_string_literal: true

class Shoes
class Para < Shoes::Drawable
class Para < Shoes::Drawable
require_relative 'font_helper.rb'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor thing, but it probably makes more sense to put the require up at the top unless it's inside a method. In this case, up top makes more sense.

@@ -0,0 +1,62 @@
module Font_helper
Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally you wouldn't use an underscore here. It would be FontHelper rather than Font_helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done : )

@@ -0,0 +1,62 @@
module Font_helper

def self.parse_font(font)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want an includable module, you'd normally define these as instance methods (e.g. "def parse_font") rather than self methods. It would be fine to have it free-standing like this and call FontHelper.parse_font, but then you don't need the includes for it. Or you could use the includes, call the methods as "parse_font" rather than "FontHelper.parse_font" and not define them with "self" in front.

@noahgibbs noahgibbs merged commit 9c18967 into scarpe-team:main Feb 8, 2024
1 check passed
@noahgibbs
Copy link
Collaborator

Thank you! Merged.

@Nj221102 Nj221102 deleted the font branch February 8, 2024 12:14
regex = /\s+(?=(?:[^']*'[^']*')*[^']*$)(?![^']*,[^']*')/
result = input.split(regex)

fs = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an excellent contribution. Thank you!

One thought, no need to take action but thinking out loud.

For readability in future it could be nice to use descriptive variable names; for what concept they are representing.

(Thinking about the "fs, foss, etc.")

Thanks again!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the advice , will keep that in mind while writing code in future : )

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