iMattPro / abbc3

Advanced BBCode Box - An extension for phpBB
https://www.phpbb.com/customise/db/extension/advanced_bbcode_box/
GNU General Public License v2.0
26 stars 28 forks source link

support for scaffoldBB #115

Closed MannixMD closed 2 years ago

iMattPro commented 2 years ago

Thanks for your PR but I can not accept it. In this state it presents a maintenance nightmare and just isn't how this should be handled. You can't just duplicate files resulting in massive duplication of Javascript and CSS styling.

The changes you are making to posting_buttons can be handled by adding a little javascript file to your style (whether it's built in to your style or added here) where you can just use jQuery to target elements and append classes to them. For examples:

$("#abbc3_buttons").addClass("my-2 row gx-0");

$("#bbcode_wizard").attr({"
    "class": "modal",
    "tabindex":"-1"
});

The changes in the CSS files... you should not be duplicating and then modifying entire CSS pages, again creating a maintenance nightmare with duplicate code going forward. For CSS files, you can create a new abbc3_posting.css in your style that imports the default ABBC3 css file, and then you just append to it the changes you need to make to the specific class or id definitions. Here's an example of how that's done from the Collapsible categories extension

MannixMD commented 2 years ago

Thank you for suggestions. It all should be ok now. One thing tho for the future please don't use id's for styling :)

iMattPro commented 2 years ago

That's much better. Are you working with the latest pull of this repo, because I did a quick test with your style and the new FONTS menu in ABBC3 really got broken by your style, because it's not a simple select menu anymore, it's now a dropdown toggle just like the Quick Links menu from the nav bar.

codecov[bot] commented 2 years ago

Codecov Report

Merging #115 (9648bf1) into master (80e132c) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #115   +/-   ##
=========================================
  Coverage     97.72%   97.72%           
  Complexity      141      141           
=========================================
  Files            11       11           
  Lines           483      483           
=========================================
  Hits            472      472           
  Misses           11       11           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 80e132c...9648bf1. Read the comment docs.

MannixMD commented 2 years ago

I need to stop spliting repos from the local environment silly me :(. Font dropdown should be fixed now :)

MannixMD commented 2 years ago

I've got a question for you about including other files. It has nothing to do with this extension but do you know if it's possible to include a html event file from all directory to another one? Basically i need to load the content from an event file from all directory into the same event file under "scaffoldBB" directory and then below add my modification is that possible? So it's like you mentioned earlier with css but for html

iMattPro commented 2 years ago

I don't think so. Have you tried it to see?

MannixMD commented 2 years ago

Yeah i did but no luck even with absolute path

iMattPro commented 2 years ago

I tried this out. The font menu dropdown was not working, giving an i.popper error in the console.

MannixMD commented 2 years ago

Weird, it works for me just fine no errors (2) Board33 - Post a reply localhost b76989e76f01 (2) Board33 - Post a reply localhost ad2ca9974838 Maybe clear cache on your test board

iMattPro commented 2 years ago

Weird, it works for me just fine no errors (2) Board33 - Post a reply localhost b76989e76f01 (2) Board33 - Post a reply localhost ad2ca9974838 Maybe clear cache on your test board

Still not working. Only works if I add popper to your style

<script src="https://cdn.jsdelivr.net/npm/@popperjs/core@2.10.2/dist/umd/popper.min.js" integrity="sha384-7+zCNj/IqJ95wo16oMtfsKbZ9ccEh31eOz1HGyDuCQ6wgnyJNSYdrPa03rtR1zdB" crossorigin="anonymous"></script>
MannixMD commented 2 years ago

I just grabbed the master branch and installed it on my board and it's working fine. You can check it here

MannixMD commented 2 years ago

ok i found the issue. When Allow usage of third party content delivery networks is set to yes it's not working

MannixMD commented 2 years ago

Just send a new style revision for validation with the fix :)