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: Added option to set gradient backgrounds #481

Merged
merged 10 commits into from
Apr 1, 2023

Conversation

AndroiableDroid
Copy link
Contributor

@AndroiableDroid AndroiableDroid commented Mar 30, 2023

Description

Fixes #201 - Half

Type of change

  • Bug fix (added a non-breaking change which fixes an issue)
  • New feature (added a non-breaking change which adds functionality)
  • Updated documentation (updated the readme, templates, or other repo files)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Tested locally with a valid username
  • Tested locally with an invalid username
  • Ran tests with composer test
  • Added or updated test cases to test new features

Checklist:

  • I have checked to make sure no other pull requests are open for this issue
  • The code is properly formatted and is consistent with the existing code style
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Screenshots

image

Copy link
Owner

@DenverCoder1 DenverCoder1 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 working on this feature 👍

I have a few comments regarding the implementation.

The demo site seems to have multiple bugs introduced regarding adding and deleting advanced properties and defaults not getting filtered. If it's too complicated to get it to work, it's fine if the demo site is updated in a separate PR. Same PR is also fine if its fully working.

(Also, I'd prefer force-pushing to be avoided since it makes it harder to track progress on the PR and with syncing changes)

README.md Outdated Show resolved Hide resolved
src/card.php Outdated Show resolved Hide resolved
tests/expected/test_card.svg Outdated Show resolved Hide resolved
 * background=DEG,COLOR1,COLOR2,....COLORN

Signed-off-by: Mohd Faraz <[email protected]>
@AndroiableDroid
Copy link
Contributor Author

Thanks for working on this feature +1

I have a few comments regarding the implementation.

The demo site seems to have multiple bugs introduced regarding adding and deleting advanced properties and defaults not getting filtered. If it's too complicated to get it to work, it's fine if the demo site is updated in a separate PR. Same PR is also fine if its fully working.

(Also, I'd prefer force-pushing to be avoided since it makes it harder to track progress on the PR and with syncing changes)

Check new commit, is everything fine

@DenverCoder1
Copy link
Owner

The backend looks great! Thanks for the continued progress 👍

Is it fine with you if I add some commits to fix some issues with the demo site?
(some old features are no longer working as they should)

@AndroiableDroid
Copy link
Contributor Author

The backend looks great! Thanks for the continued progress 👍

Is it fine with you if I add some commits to fix some issues with the demo site? (some old features are no longer working as they should)

Ya why not, which features?

@DenverCoder1
Copy link
Owner

DenverCoder1 commented Mar 30, 2023

Here are some things I noticed

  • When choosing a color property to add, it seems to add the wrong property (not the one selected)
  • After adding background, it seems that changing other fields no longer updates the image
  • If fields are set to the default value, they can be removed from the url to make it shorter but it seems like that doesn't happen
  • JSON format shows a broken image instead of a codeblock
  • The "Export to PHP" doesn't work with background because of the new format
  • Maybe there should be an option to change the background as a solid color instead of a gradient?

I'm refactoring some stuff and I'll probably fix most of these along the way. 👍

@AndroiableDroid
Copy link
Contributor Author

Here are some things I noticed

  • When choosing a color property to add, it seems to add the wrong property (not the one selected)
  • After adding background, it seems that changing other fields no longer updates the image
  • If fields are set to the default value, they can be removed from the url to make it shorter but it seems like that doesn't happen
  • JSON format shows a broken image instead of a codeblock
  • The "Export to PHP" doesn't work with background because of the new format
  • Maybe there should be an option to change the background as a solid color instead of a gradient?

I'm refactoring some stuff and I'll probably fix most of these along the way. 👍

Do I need to see them or you are handling?

@AndroiableDroid
Copy link
Contributor Author

Looks great.

@DenverCoder1
Copy link
Owner

DenverCoder1 commented Mar 30, 2023

Almost everything looks like it's pretty good right now.

One thing left I can think to consider is whether we should allow users to still set background colors on the demo site without controlling both a start and end color. For example, maybe there could be a radio input to select between solid color and gradient for the background. Possibly it could be two separate menu items, "background (solid)" and "background (gradient)", and adding one would disable the other from being chosen. I'm not entirely sure what's best.

What do you think about this?

List of things to do:

  • feature - Add a way to choose a color without using gradient controls (see above) - will be done in another PR
  • fix - PNG mode doesn't have transparency when colors contain opacity
  • fix - "Open Permalink" button on demo site doesn't prefill the angle and colors from the url

Copy link
Owner

@DenverCoder1 DenverCoder1 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution 🎉

@DenverCoder1 DenverCoder1 changed the title feat: Added gradient background tuner feat: Added option to set gradient backgrounds Apr 1, 2023
@DenverCoder1 DenverCoder1 merged commit e1f4836 into DenverCoder1:main Apr 1, 2023
2 checks passed
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.

New feature - gradient background & text shadow
2 participants