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

Added style parameter #707

Closed
wants to merge 4 commits into from
Closed

Added style parameter #707

wants to merge 4 commits into from

Conversation

ricardoboss
Copy link
Collaborator

This PR will partially fix #705.

Why partially? Because badges/poser doesn't support the fourth requested style: for-the-badge. There is already an issue for this: badges/poser#55. I will try and implement this over there as well.

@JellyBellyDev
Copy link
Member

@ricardoboss thanks so much for your PR! What do you think to add also the specific section on the landing page to explain the new feature?

@ricardoboss
Copy link
Collaborator Author

Sounds reasonable. I will add some documentation to this PR.

@ricardoboss
Copy link
Collaborator Author

@JellyBellyDev I need some help adding the style parameter to the routes. What I did does not seem to work... Can you take a look?

// }

var_dump($style);
die;
Copy link
Member

Choose a reason for hiding this comment

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

die

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for testing. I am not getting the actual style parameter I put in the URL. It does not work yet! Can you help?

Copy link
Member

Choose a reason for hiding this comment

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

after making the change, restart docker-compose, sometimes the changes are not applied in the container :S

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already did. I am not too familiar with Symfony yet, maybe I just made a stupid mistake somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

as soon as I have a second I throw an eye too ;)

Copy link
Member

Choose a reason for hiding this comment

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

I fixed your problem! Can i commit on this branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

Copy link
Member

Choose a reason for hiding this comment

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

I add my commit here: #733

{% endfor %}
</div>
</div>
</section>
Copy link
Member

Choose a reason for hiding this comment

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

can you put a screenshot please! :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do once I get the correct badges rendered :)

Copy link
Member

Choose a reason for hiding this comment

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

WDYT? :)

Schermata 2022-02-18 alle 18 12 07

Copy link
Member

Choose a reason for hiding this comment

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

OR
Schermata 2022-02-18 alle 19 06 33

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First the flat variants, then plastic and for-the-badge. Speaking of, can you update PUGX/poser to also include the fourth style, please? :D

Copy link
Member

Choose a reason for hiding this comment

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

I tried to do composer update package but I've a error! :S

Copy link
Member

Choose a reason for hiding this comment

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

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires badges/poser ^2.3 -> satisfiable by badges/poser[v2.3.0].
    - badges/poser v2.3.0 requires kartsims/easysvg ^2.0 -> found kartsims/easysvg[dev-master, 1.0] but it does not match the constraint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to add my fork as a repository as well, it seems...

    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/ricardoboss/easysvg"
        }
    ],

@JellyBellyDev
Copy link
Member

JellyBellyDev commented Feb 18, 2022

Hi @ricardoboss,
I tagged a new feat on https://github.com/badges/poser.
Can you rebase with master and update this PR? Thanks

@ricardoboss
Copy link
Collaborator Author

@JellyBellyDev done

@JellyBellyDev
Copy link
Member

@ricardoboss shouldn't I also notice a diff to composer.lock?

@ricardoboss
Copy link
Collaborator Author

No, because I haven't updated the PUGX/poser package yet.

@ricardoboss
Copy link
Collaborator Author

@JellyBellyDev do you want me to close this PR in favor of #733?

@JellyBellyDev
Copy link
Member

Yea! Thanks! in doing so we can work together! ;)

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.

Add a way for different styled badges
3 participants