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

Oriol.sprite fun #320

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Oriol.sprite fun #320

wants to merge 16 commits into from

Conversation

osenan
Copy link
Contributor

@osenan osenan commented Nov 16, 2020

This includes two new functions addsvg_sprite and rmsvg_sprite that add the functionality to easily edit sprite maps, add or remove svgs to the sprite map. It is motivated by this issue:

#226

To read, parse and edit the svg files it is necessary the xml2 package, so I have edited the DESCRIPTION and added as suggested packages. Documentation and tests have also been added.

DoD

  • Major project work has a corresponding task. If there’s no task for what you are doing, create it. Each task needs to be well defined and described.

  • Change has been tested (manually or with automated tests), everything runs correctly and works as expected. No existing functionality is broken.

  • No new error or warning messages are introduced.

  • All interaction with a semantic functions, examples and docs are written from the perspective of the person using or receiving it. They are understandable and helpful to this person.

  • If the change affects code or repo sctructure, README, documentation and code comments should be updated.

  • All code has been peer-reviewed before merging into any main branch.

  • All changes have been merged into the main branch we use for development (develop).

  • Continuous integration checks (linter, unit tests) are configured and passed.

  • Unit tests added for all new or changed logic.

  • All task requirements satisfied. The reviewer is responsible to verify each aspect of the task.

  • Any added or touched code follows our style-guide.

@osenan osenan requested a review from jchojna November 16, 2020 03:02
@osenan osenan self-assigned this Nov 16, 2020
R/sprite.R Outdated Show resolved Hide resolved
R/sprite.R Outdated Show resolved Hide resolved
R/sprite.R Outdated Show resolved Hide resolved
R/sprite.R Outdated Show resolved Hide resolved
Copy link
Contributor

@jchojna jchojna left a comment

Choose a reason for hiding this comment

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

Good work 💯 :) It would be also great to have some simple script such that one you sent me on slack, in examples folder, to demonstrate functionality of these functions

* Change function and variable names to fit with coding style:
	- addsvg_sprite for add_svg_sprite
	- rmsvg_sprite for remove_svg_sprite
	- idExists for id_exists

* Simplify code in id_exists return statement, easier to read and mantain

* Update tests and documentation according to changes in functions
@osenan osenan requested a review from jchojna December 16, 2020 05:07
@osenan
Copy link
Contributor Author

osenan commented Dec 16, 2020

Thank you for your comments. Finally I have adressed all changes. Only instead of putting one app in examples I have put more examples in the function directly. I did it this way because these new functions are helper functions and are not directly components of a Shiny App. Building an example app does not show its functionality, while showing more examples in documentation it does.

@jchojna
Copy link
Contributor

jchojna commented Dec 16, 2020

Great work, tested it and the sprite map gets generated and works fine! The only thing I miss is some UI function to add svg DOM node. An example function:

svg_icon <- function(path_to_sprite_map, id, class = "", style = "") {
    HTML(glue::glue("
        <svg
            class = '{class}',
            style = '{style}',
            viewBox = '0 0 <calculated width> <calculated height>'
        >
            <use href = '{path_to_sprite_map}#{id}'></use>
        </svg>"
    ))
}

svg_icon("temp.svg", "appsilon_logo", "test-svg", "fill: red;")

I didn't test this function, but briefly this is how I would imagine that

@osenan
Copy link
Contributor Author

osenan commented Dec 17, 2020

Hi @jchojna I agree with you that to close this issue we could add a function to insert sprite maps easily on the UI. Consequently we should also write an example app using it.

Copy link
Contributor

@jchojna jchojna left a comment

Choose a reason for hiding this comment

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

I set the conversations as resolved, as the updates have been added. LGTM

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