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

feat: Add support for missing SVGs #3962

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ChinoUkaegbu
Copy link

@ChinoUkaegbu ChinoUkaegbu commented Sep 20, 2024

Closes #3954

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

After these steps, you're ready to open a pull request.

a. Give a descriptive title to your PR.

b. Describe your changes.

c. Put `closes #XXXX` in your comment to auto-close the issue that your PR fixes (if such).

@ElijahAhianyo
Copy link
Collaborator

ElijahAhianyo commented Sep 20, 2024

Hi @ChinoUkaegbu , thanks for looking into this, can you provide some examples to test with? Will also be nice to add components for SVG radial_gradient and SVG text

@ChinoUkaegbu
Copy link
Author

Sure thing! Just to clarify, do you mean unit tests (and if so where should I add them?) or just examples of the SVGs in use

@ElijahAhianyo
Copy link
Collaborator

Yes, just examples in the PR description will do

@Lendemor
Copy link
Collaborator

Lendemor commented Sep 20, 2024

If you can, add some basic unit tests in tests/components/el/test_svg.py

They don't have to be too complicated, but at least check that create() call work, and that the result of .render() has the property "name" with the correct value.

You can use test/components/recharts/test_cartesian.py as a reference.

@ChinoUkaegbu
Copy link
Author

Code for the examples:

def index() -> rx.Component:
    # Welcome Page (Index)
    return rx.container(
        rx.color_mode.button(position="top-right"),
        rx.vstack(
            rx.heading("Examples", size="9"),
            rx.hstack(
                rx.text("Ellipse Example", size="5"),
                rx.el.svg(rx.el.svg.ellipse(cx="100", cy="50", rx="60", ry="40")),
            ),
            rx.hstack(
                rx.text("Line Example", size="5"),
                rx.el.svg(rx.el.svg.line(x1="50", x2="200", y1="80", y2="90")),
            ),
            
            spacing="5",
            justify="center",
            min_height="85vh",
        ),
        rx.logo(),
    )

Images:
Screenshot (378)

For the line component, it was only visible when I added stroke="black" in the console. Otherwise it was rendered but just wasn't visible
Screenshot (379)

Does it make sense then to have stroke be a parameter defined in media.py?

@ChinoUkaegbu
Copy link
Author

I'll work on radial_gradient, text and the tests in the meantime!

@Lendemor
Copy link
Collaborator

stroke is a CSS props, so I don't think it's needed as an explicit props for now.
Everything seems good, just need to refresh the pyi files to pass the CI 👍

@ChinoUkaegbu
Copy link
Author

ChinoUkaegbu commented Sep 26, 2024

Also just wondering if there is a reason why bool is one of the types allowed for the coordinates in linearGradient?
https://github.com/reflex-dev/reflex/blob/main/reflex/components/el/elements/media.py#L385-L394

from the MDN docs it seems as though the value type is <length-percentage> | <number>

@ChinoUkaegbu
Copy link
Author

Code for the radial gradient and text examples:

rx.hstack(
  rx.text("Radial Gradient Example", size="5"),
  rx.el.svg(
      rx.el.svg.defs(
          rx.el.svg.radial_gradient(
              rx.el.svg.stop(offset="10%", stop_color="gold"),
              rx.el.svg.stop(offset="95%", stop_color="red"),
              id="my_rad_grad", cx="50%", cy="50%", r="50%", fx="50%", fy="50%"
          )
      ),
      rx.el.svg.ellipse(cx="100", cy="50", rx="60", ry="40", fill="url('#my_rad_grad')")
  )
),
rx.hstack(
  rx.text("Text Example", size="5"),
  rx.el.svg(rx.el.svg.text("This is the example", x="5", y="15", rotate="30"))
),

Image:
Screenshot (388)

@picklelo
Copy link
Contributor

Also just wondering if there is a reason why bool is one of the types allowed for the coordinates in linearGradient? https://github.com/reflex-dev/reflex/blob/main/reflex/components/el/elements/media.py#L385-L394

from the MDN docs it seems as though the value type is |

I think this may be a copy+paste oversight, we should match the MDN docs

Copy link
Contributor

@picklelo picklelo 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 adding!

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.

Support missing svg elements
4 participants