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

Replace react fonticonpicker #18

Closed
wants to merge 6 commits into from

Conversation

drzraf
Copy link
Contributor

@drzraf drzraf commented Feb 24, 2023

Fixes #4

For convenience and avoiding conflict, it includes / is based on top of #17 but only 27f4606 is a concern for this MR.

(I could also create a MR specific to this feature if really needed)

NB: Feel free to change/test/enhance the MR at your needs.

@drzraf
Copy link
Contributor Author

drzraf commented Mar 8, 2023

ping

@aniketcoolplugins007
Copy link
Collaborator

Hi @drzraf,

Thanks for your contribution.
During development we have added this code to dev branch. Now I have merged the dev branch into master.
Soon we will release the master branch code to wordpress.org inside the timeline block plugin.

Thanks & Regards

@drzraf
Copy link
Contributor Author

drzraf commented Apr 4, 2023

Thank you for merging. Very happy this could get in.

Some comments:

  • The workflow were code is merging in a hidden way (against a hidden codebase) before being "reintregrated" in batches in dev or main isn't very friendly because.

    • A) Atomic commits / chunks are not preserved.
    • B) It's not merge-friendly (it'd be impossible to maintain multiple branches simultaneously without very painful rebases everytime).
    • C) The original commit's authors contribution/ownership isn't preserved (leaving contributors in readme.txt as the only right place left).
    • D) It's not possible to review/comment afterward (no proper MR)
  • There still also lot of whitespace/indents problems still reaching the code. Chunks like:

+                       if(value[i] > 0 || value[i] < 0){    // missing space. Replace with `value[i] !== 0`
+                               updateSettingsapply(' timeline-container_pd_apply');   // extra space in the string(?!)
+                               break;
+                       }else{ // missing spaces
+                               updateSettingsapply('');
+                       }
  • I don't understand why the horizontal layout was removed in the last batch of changes.

@aniketcoolplugins007
Copy link
Collaborator

@drzraf

We have used dev branch for development and after completion of development we have merged dev branch code to master branch.
1,2) I am new in open source development I am using different branches to add new changes if you have better workflow please suggest me.
3) There was some designing and compatibility issue in your (PR #18) so we have used your code in dev branch at the time of deprecation and we have also improved the design.
4) Thanks for your suggestion we have follow this proper MR request to next time.
5) I have improved the code.
6) Horizontal layout is not being used in the timeline block so we removed extra code.

Thanks & Regards

@narinder9
Copy link
Owner

@drzraf Many Thanks for your contributions. We have mentioned you on the WordPress.org plugin page.
https://wordpress.org/plugins/timeline-block/
I hope you are happy with it.

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.

fonticonpicker/react-fonticonpicker dep isn't compatible with react 17.x
3 participants