Skip to content

[WIP] Prototype new Text components #146

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

Closed
wants to merge 1 commit into from
Closed

Conversation

arthurxavierx
Copy link
Contributor

Just had this idea and prototyped this possible API for the new Text components. I'd like to hear your opinions, ideas and concerns on this, please.

Copy link
Member

@maddie927 maddie927 left a comment

Choose a reason for hiding this comment

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

I like this a lot. We might want a few more modifiers like color :: (Colors -> Color) -> TextModifier too.

type TextModifier = forall r. PropsModifier (component :: Text, element :: TextElement | r)

_body :: TextModifier
_body = propsModifier _{ element = R.span' :: TextElement }
Copy link
Member

Choose a reason for hiding this comment

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

We might not need to continue this underscore convention here, since the types are more descriptive (i.e. there isn't a special StyleModifier all the others depend on)

{ fontSize: S.str "14px"
, lineHeight: S.str "17px"
, margin: S.str "0"
, padding: S.str "0"
Copy link
Member

Choose a reason for hiding this comment

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

I think we want Text to default to white-space: pre; and have Paragraph override that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe pre-wrap is safer?

Copy link
Member

Choose a reason for hiding this comment

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

I think you're right. I had been thinking we might want the base Text component to never wrap and Paragraph to contain the wrapping behavior/settings (like spacing/line-height which wouldn't be available on Text), but wrapping is just so much more common that that might be more annoying than helpful.

Copy link
Member

Choose a reason for hiding this comment

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

Either way, Text and Paragraph should be setting it explicitly so we don't inherit unexpected values. We probably want to do the same with font-family, and pull both font-family and the sizing/line-height settings from the theme.

Copy link
Member

Choose a reason for hiding this comment

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

Side note, if we did make Text nowrap and Paragraph wrap, we'd probably have trouble doing that with the current api, where Paragraph's children are Texts (or other components with Text children) as the inner Text nowrap would be more specific. That probably means we have the same issue with other styles here.. Paragraph's settings might need to be marked important, unless you can think of a better option.

Copy link
Member

@maddie927 maddie927 May 6, 2020

Choose a reason for hiding this comment

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

I see paragraph_ is partially working around that here, but paragraph would be dependent on each child Text having its own sizing/line-height set. Hmm. I think.. Paragraph defaults and overrides should override all child Text defaults, but not child Text overrides, i.e.

paragraph
  $ emphasize
  $$$
    -- paragraphs are not emphasized by default but this one has a modifier
    -- but this local override still takes priority
    [ text $ nonEmphasized $$$ "hello"
    -- paragraphs have default text size and line height, but this local override
    -- would still take priority.
    -- this text inherits the paragraph emphasis setting
    , text $ fontSize 20 $$$ "goodbye"
    ]

Copy link
Member

Choose a reason for hiding this comment

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

These problems are kind of unique to Paragraph and Text

Copy link
Contributor Author

@arthurxavierx arthurxavierx May 8, 2020

Choose a reason for hiding this comment

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

Either way, Text and Paragraph should be setting it explicitly so we don't inherit unexpected values. We probably want to do the same with font-family, and pull both font-family and the sizing/line-height settings from the theme.

Yes, excellent points!

Side note, if we did make Text nowrap and Paragraph wrap

I don't think we should make Text nowrap. What would be the reason for doing that?

Paragraph defaults and overrides should override all child Text defaults, but not child Text overrides, i.e.

Maybe we could get around that by having all text defaults be inherit and setting those defaults globally? But then that'd kind of defeat the purpose of having LumiTheme :p

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should make Text nowrap. What would be the reason for doing that?

I think I agree, it was an idea I inherited from another api but it doesn't give us anything here.

Maybe we could get around that by having all text defaults be inherit and setting those defaults globally? But then that'd kind of defeat the purpose of having LumiTheme :p

Maybe.. it might be risky if there are non-lumi-components styles used anywhere on the page

@@ -0,0 +1,194 @@
module Lumi.Components2.Text where
Copy link
Member

Choose a reason for hiding this comment

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

We might not want to expose mkHeaderComponent just for consistency. Do you think the Text constructor should be exposed (don't know, just curious)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I forgot to constrain the exports 😅

paragraph_ :: LumiComponent TextProps
paragraph_ = paragraph <<< renderInnerText
where
renderInnerText :: Setter' (LumiProps ParagraphProps) (LumiProps TextProps)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this isn't being used with any lens functions, I feel like it might be easier to read if you expanded this Setter' type synonym.

]
}

paragraph :: LumiComponent ParagraphProps
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure what's going on here. paragraph seems to ignore its element prop and always use p' instead, but paragraph_ doesn't?

@arthurxavierx arthurxavierx marked this pull request as draft May 8, 2020 13:56
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