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

Dev #14

Merged
merged 4 commits into from
Jan 3, 2023
Merged

Dev #14

merged 4 commits into from
Jan 3, 2023

Conversation

aniketcoolplugins007
Copy link
Collaborator

New settings integration and content update

content update and new setting integration
Net settings integration and content update
Icon position, arrow position middle line position issue fix after increasing the icon and middle line size
add some styling
@narinder9 narinder9 merged commit 8c504dc into main Jan 3, 2023
@drzraf
Copy link
Contributor

drzraf commented Jan 4, 2023

Dears,

Aside from the improvements this PR adds (and I couldn't test yet), as a simple external contributor I think that this PR misses many key points of open-source development:

  • Provided as one enormous unreviewable changeset instead of a bunch of small, atomic and self-contained commits
  • No comment explaining the changes
  • A lot of whitespace changes
  • We don't know if this code update is the result of a careful development or an override from a local developer copy not correctly synchronized with latest Git
  • Strange javascript constructs (templated encapsulation of ternary operator)
  • Apparent removal of feature previously introduced ("Alternating Sided") without explanation (both sided allowed full control on each item what alternating sided doesn't. Each of them matched a distinct but valid use-case)
  • readme.md/txt files now contains HTML
  • Also please note that as long as do not rely on <style scoped="true"> #8 isn't fixed, any inline-style change is going to break the timeline on all installations

@aniketcoolplugins007
Copy link
Collaborator Author

aniketcoolplugins007 commented Jan 5, 2023

Hi Drzraf

  1. Thanks for your suggestion. we will remember this in future and commit small changes.
    2 && 3) Sorry I forgot to add all the comments to the new changes, I add it now and also we will fix whitespace.
  2. I have pull latest code from GitHub repository.
    5 && 7) We will fix this issue in next version.
  3. Alternating Sided will work only for both sided not one sided so we can use alternating sided inside the both sided layout but we have provided a switcher option to add alternating sided.
  4. Scoped="true" it specifies that the styles only apply to this element's parent element and that element's child elements.

@narinder9
Copy link
Owner

Hi @drzraf ,
May the New Year 2023 bring you more happiness, success, love, and blessings!

Thanks for detecting and mentioning these issues.
@aniketcoolplugins007 is our team member. He is a beginner in development.
Yes, he has done many mistakes. Please guide him. Now he will work full-time on this plugin.

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.

None yet

3 participants