[rrssb] Ridiculously Responsive Social Share Buttons [#2807809]

My main priority and focus is on the new 2.x version of the code. I would like to see v8.2.x. I don't think there is any need for v8.1.x. 7.2.x is still new and in beta, so if you see any problems please let me know.

Log in or register to post comments

Comment #6

himanshu-dixit commented 2 February 2017 at 11:56

Issue summary:

View changes

Log in or register to post comments

Comment #7

himanshu-dixit commented 2 February 2017 at 11:57

@AdamPS Sorry, took some time. So currently modules functionality is remaining, and i have ported the configuration form. Please review the form and check if it is working. I am planning to do the documentation in the last. Here is the sandbox that i created, https://www.drupal.org/sandbox/himanshu-dixit/2848939

Log in or register to post comments

Comment #8

adamps commented 3 February 2017 at 18:43

Status:

Active

» Needs work

Great, that's a good start. I created a 8.x.2.x dev branch and copied your sandbox in. Next, I did some code review and testing, and checked in another version with some fixes. Please take a look at what I changed, and ask me if you think I made a mistake or can't understand why I changed something.

I have made a 8.x.2.x dev release. The admin form now runs. I am seeing one error after doing drush cr that I suspect may come from rrssb_library_info_build:

Warning: Invalid argument supplied for foreach() in Drupal\Component\Utility\NestedArray::mergeDeepArray() (line 327 of core/lib/Drupal/Component/Utility/NestedArray.php).

If you have more time, next step you could start work on getting the buttons to display.

I have made you a maintainer with permission to check in to VCS. Please only write to D8 version for now.

Log in or register to post comments

Comment #9

adamps commented 3 February 2017 at 18:45

Issue summary:

View changes

Log in or register to post comments

Comment #10

denutkarsh commented 5 February 2017 at 20:41

Status:

Needs work

» Needs review

Status	File	Size
new	Test.png	26.91 KB

@AdamPS I have made some other changes, and now the module displays the button using Blocks, Entities (Nodes) . I also fixed some coding standards in the latest commit. I haven't checked `drush cr` but i would check it soon. I placed the rrssb-plus in /libraries directory in the root to make this module work. Thanks for the changes you committed :)

Edit: Sorry, looks Like My Brother's Drupal Account was logged in.

Log in or register to post comments

Comment #11

denutkarsh commented 5 February 2017 at 20:23

Issue summary:

View changes

Log in or register to post comments

Comment #12

himanshu-dixit commented 5 February 2017 at 20:40

Status:

Needs review

» Needs work

@AdamPS Anyways, i guess we still need to solve many errors described here https://pareview.sh/node/986 . Please review, the current 8.x-2.x branch.
Thanks

Log in or register to post comments

Comment #13

adamps commented 23 March 2017 at 10:59

Issue summary:

View changes

OK, nice work. We are making great progress here. As before I have committed a bunch of bug fixes and tidy up. Please let me know if you have any questions or concerns.

Bugs

Buttons not lined up correctly, js not working.
Prefix is printed twice.

Testing

We definitely need to test with all caching and aggregation turned on.

Outstanding work

There are two @@ marks in the code that are TODO.

I am happy to look at features such as https://www.drupal.org/node/2827868 but I suggest let's get the existing function working first, then I can make a first alpha release.

In terms of pareview.sh mostly I think they are not errors, just minor warnings and tidy up. It's not my top priority but it is something we can work on. However I would prefer to avoid making extra work and inconvenience. For example I don't see that it matters if lines are longer than 80 characters these days as we have big widescreen displays. Maybe the code is easier to read if it spreads across the full screen? It could also help to keep D8 closer to D7.

Log in or register to post comments

Comment #14

adamps commented 23 March 2017 at 10:59

I have released a new alpha which fixes the two bugs from #13. After upgrade to that release you must also update the rrssb-library.

@himanshu-dixit for your comment in #12, I have fixed a few of them. For the rest:

I would prefer to leave "Line exceeds 80 characters".
You are welcome to fix return/parameter comments.
The last two in RRSSBSettingsForm.php are possibly more subtle, so if fixing them please test it carefully.

Remaining priority item is to test with caching and aggregation turned on. I would appreciate any help with that.

Once that is done, I'd welcome @himanshu-dixit to start work on #2827868: Support multiple buttons sets (for example 1 for Share + 1 for follow us).

Log in or register to post comments

Comment #15

adamps commented 23 March 2017 at 11:29

Caching should be working now.

D8 version is quite close to being ready. The main item missing is migration code from D7->D8.

Log in or register to post comments

Comment #16

adamps commented 26 September 2017 at 07:33

Assigned:	himanshu-dixit	» Unassigned
Status:	Needs work	» Reviewed & tested by the community

RC now available, API can be considered stable.

Still missing migration from D7 #2911705: Migration from D7->D8 otherwise D8 version is complete.

Log in or register to post comments

Comment #17

mmjvb commented 18 November 2017 at 22:00

Issue summary:

View changes

Added 8.x-2.0-rc4 and stable release blocker.

Log in or register to post comments

Comment #18

adamps commented 19 November 2017 at 07:38

Issue summary:

View changes

Yes we should keep the summary up to date. Note that the there is a template that explains how to fill in the different sections.

Log in or register to post comments

Comment #19

mmjvb commented 19 November 2017 at 14:16

Sounds like you don't appreciate my contribution. For the record, see no justification in the template to remove my edits.

Log in or register to post comments

Comment #20

adamps commented 19 November 2017 at 16:44

I thought that I had worded my #18 to sound positive and that between us we have made things better.

The template for "Where is the code?" does not include a link to a specific release. I see a very good reason for that because before long it will be out of date when a newer release comes along.
In my view (as current maintainer) #2911705: Migration from D7->D8 is not a stable release blocker. However I agree it's good to have a link so I moved it to the summary.

Log in or register to post comments

Comment #21

mmjvb commented 19 November 2017 at 17:16

Didn't sound positive to me, came across as sarcastic. Provided the link to the current version and update it when appropriate. Have done that at quite a number of issues. Definitely beats the links found there, including the one there now. It is more of a convenience for those interested to try it based on information found in the issue. The template allows for all kind of links and doesn't disallow links to specific versions.

Good to see that as a maintainer you did put in some effort to keep this current. It is your own comment that gave me the impression that there was release blocker. Wonder now what point you were trying to make if it is not a release blocker.

It is a shame to see such poor understanding of release management with module maintainers. Can only remember to have seen one good example of a roadmap. Unfortunately, can't remember which module. Also a pity that the template doesn't provide a proper description. Obviously, the roadmap should tell what to expect when. Allowing you to phase that any way you want. As long as there is a start and finish, finish being the Stable release. With a proper roadmap, nobody would have perceived there was a release blocker. When can we expect a stable release?

Log in or register to post comments

Comment #22

adamps commented 19 November 2017 at 17:28

The module is provided free in my spare time and you can choose to use it or not.

@mmjvb I expect most readers would judge that my comments have been harmless/polite and yours increasingly hostile. Please don't comment any more on this issue or anywhere else on RRSSB.

Drupal Code of Conduct

Log in or register to post comments

Comment #23

mmjvb commented 26 November 2017 at 23:23

@AdamPS Suggest you read that yourself. Looks like you forgot what this issue is about. Just like you, don't like to be told what to do. Difference with you is I am pointing out your mistakes.

Expect most readers to agree with me, that is apart from those maintainers that make those same mistakes and don't want to learn from them.
Fortunately, this issue is in a project outside of your control. Being the maintainer of the module concerned doesn't give you the benefits you have in the issue queue of that module. Obviously, with such a poor attitude of a maintainer I have no interest in your modules.
This issue is about the port of the module to D8, which is why I monitor it, and will continue to do that despite you not liking it. But rest assured, won't be wasting my time on you.