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 button for exporting a theme from the demo site #176

Merged
merged 9 commits into from
Oct 8, 2021
Merged

feat: Add button for exporting a theme from the demo site #176

merged 9 commits into from
Oct 8, 2021

Conversation

komen205
Copy link
Contributor

@komen205 komen205 commented Oct 5, 2021

Description

Hey! I created this working solution, please let me know if it's appropriated. This is not a final solution.

I created json_decode.php file so JS sends the properties json and php exports it to the text area.
It was the only working solution that I could make it work.
HTML and CSS is not final, please let me know where you want me to export it.

Let me know what changes I can make and if I'm going the right path.

Fixes #144

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
  • [X ] My changes generate no new warnings

Screenshots

image

@komen205
Copy link
Contributor Author

komen205 commented Oct 5, 2021

I closed #152 because I deleted the branch, lets work on this one.

Comment on lines 183 to 194
let properties = [
"border",
"stroke",
"ring",
"fire",
"currStreakNum",
"sideNums",
"currStreakLabel",
"sideLabels",
"dates",
"background",
];
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of listing them here, can just use a more specific query when creating the mapping below

Eg.

document.querySelectorAll(".advanced .param.jscolor")

instead of

document.querySelectorAll(".param")

Comment on lines 195 to 211
let array = Array.from(document.querySelectorAll(".param")).reduce(
(acc, next) => {
let obj = { ...acc };
let value = next.value;
if (value.indexOf("#") >= 0) {
// if the value is colour, remove the hash sign
value = value.replace(/#/g, "");
if (value.length > 6) {
// if the value is in hexa and opacity is 1, remove FF
value = value.replace(/(F|f){2}$/, "");
}
}
obj[next.id] = value;
return obj;
},
{}
);
Copy link
Owner

Choose a reason for hiding this comment

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

This is same code from line 10, if it's used twice, it should be extracted to a function to use in both places.

Eg.

function objectFromElements(elements) {
  // create a key value mapping of parameter values from all elements in a Node list
  return Array.from(elements).reduce((acc, next) => {
    let obj = { ...acc };
    let value = next.value;
    if (value.indexOf("#") >= 0) {
      // if the value is colour, remove the hash sign
      value = value.replace(/#/g, "");
      if (value.length > 6) {
        // if the value is in hexa and opacity is 1, remove FF
        value = value.replace(/(F|f){2}$/, "");
      }
    }
    obj[next.id] = value;
    return obj;
  }, {});
}
// line 10
const params = objectFromElements(document.querySelectorAll(".param"));

// here
const params = objectFromElements(document.querySelectorAll(".advanced .param.jscolor"));

Comment on lines 212 to 218
var exportPhp = {};

for (const [key, val] of Object.entries(array)) {
if (properties.includes(key) > 0) {
exportPhp[key] = val;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Using the above changes, this is unnecessary

.map((key) => `\t"${key}" => "#${exportPhp[key]}",\n`)
.join("") +
"]";
document.getElementById('test').value = output;
Copy link
Owner

Choose a reason for hiding this comment

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

I assume the "test" and "button" text are temporary, but I'll mention anyway, they should be made more descriptive.

Comment on lines 100 to 101
<button class="btn" type="button" onclick='return exportPhp()'>button</button>
<textarea id="test"></textarea>
Copy link
Owner

Choose a reason for hiding this comment

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

This can go inside the advanced panel and you can make the textarea hidden until the export button is actually clicked.

@komen205
Copy link
Contributor Author

komen205 commented Oct 7, 2021

What a much simpler solution.
I don't even know how I didn't think about getting the properties using JS instead of declaring them.

👍 💯
Let me know how do you want the css @DenverCoder1

@komen205 komen205 requested a review from DenverCoder1 October 7, 2021 20:14
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.

CSS for textarea to be below button, full-width, initially 100px height, and only resizable vertically

textarea#exportedPhp {
    display: block;
    margin-top: 10px;
    width: 100%;
    resize: vertical;
    height: 100px;
}

@komen205 komen205 requested a review from DenverCoder1 October 8, 2021 10:07
@DenverCoder1
Copy link
Owner

Looks great! 🎉 Thanks again for your contributions!

@DenverCoder1 DenverCoder1 changed the title Add button for exporting a theme from the demo site feat: Add button for exporting a theme from the demo site Oct 8, 2021
@DenverCoder1 DenverCoder1 merged commit 7e73876 into DenverCoder1:main Oct 8, 2021
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 button for exporting a theme from the demo site
2 participants