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

[ticket/12291] Allow extensions to use custom topic icons and smilies #3748

Closed
wants to merge 9 commits into from

Conversation

@rxu
Copy link
Contributor

commented Jul 7, 2015

@Nicofuma

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

Can you add a test case please?

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2015

Yes, but I'm currently guessing what kind of test could it be.

TODO: add tests.

@Nicofuma Nicofuma added the 3.2 (Rhea) label Jul 8, 2015

@Nicofuma Nicofuma added this to the 3.2.0 milestone Jul 8, 2015

@rxu rxu force-pushed the rxu:ticket/12291 branch from e6af481 to 7dec7ff Jul 8, 2015

@phpbb-user phpbb-user added the WIP 🚧 label Jul 8, 2015

@rxu rxu force-pushed the rxu:ticket/12291 branch from 7dec7ff to a69ffd3 Jul 27, 2015

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2015

This also requires changes to the textformatter @s9e.

@JoshyPHP

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

@rxu What changes?

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2015

@s9e F.e. here https://github.com/phpbb/phpbb/blob/master/phpBB/phpbb/textformatter/s9e/renderer.php#L144
$this->set_smilies_path($root_path . $config['smilies_path']); to $this->set_smilies_path($root_path);, dunno if there're another places regarding smilies and icons in the textformatter (didn't find it so far).

@JoshyPHP

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

I'm not familiar with this PR and how it relates to smilies. From what I see it removes the dynamic smilies_path from images. In that case you'd need to remove the following methods:

  • phpbb\textformatter\renderer_interface::set_smilies_path()
  • phpbb\textformatter\s9e\renderer::configure_smilies_path()
  • phpbb\textformatter\s9e\renderer::set_smilies_path()

Remove {$T_SMILIES_PATH}/ from the template set in phpbb\textformatter\s9e\factory, and remove the configure_smilies_path call from config/default/container/services_text_formatter.yml.

@rxu I can send you a PR if you want.

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2015

@s9e Yeah, the idea is to dump the full relative path to icons and smilies to the corresponding DB columns (smiley_url/icons_url) to allow using icons/smilies from extensions. But acp_icons module will still be looking for core icons/smilies in smilies_path/icons_path to write the path to the db.

I can send you a PR if you want.

That would be nice, thanks. Although I'm not sure if smilies path configuration should be removed fully from the renderer, as it should be pointed to the root path anyway, otherwise smilies won't be displayed as far as I see because of not prepended with the board URL.

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2015

Note: this PR will be conflicting with #3799 as the latter removes /phpbb/install/schemas/schema_data.sql.

@rxu rxu force-pushed the rxu:ticket/12291 branch from 0dbe44f to e4affe4 Jul 31, 2015

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2015

#3748 rebased onto the latest master branch.

@rxu rxu force-pushed the rxu:ticket/12291 branch 3 times, most recently from 41118f6 to eddece2 Oct 7, 2015

@Nicofuma Nicofuma modified the milestones: 3.2.0-a1, 3.2.0-a2 Oct 17, 2015

@rxu rxu force-pushed the rxu:ticket/12291 branch from eddece2 to 9d2aa15 Nov 3, 2015

@rxu rxu force-pushed the rxu:ticket/12291 branch 3 times, most recently from 9ba964f to 601bfd7 Dec 7, 2015

@Nicofuma Nicofuma modified the milestones: 3.2.0-RC1, 3.2.0-RC2 May 2, 2016

@rxu rxu force-pushed the rxu:ticket/12291 branch from 326c838 to bd33713 Aug 28, 2016

@rxu rxu force-pushed the rxu:ticket/12291 branch 2 times, most recently from 019b586 to 55a63ac Sep 30, 2016

@rxu rxu force-pushed the rxu:ticket/12291 branch from 55a63ac to 4555ba1 Oct 3, 2016

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2016

Actually I'm not sure anymore if this option is really needed in 3.2+.

@marc1706

This comment has been minimized.

Copy link
Member

commented Oct 3, 2016

Why do you think that? Could you elaborate on that?

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2016

Well, with textformatter supporting Emoji there's very small chance for extension using custom smilies will appear as using Emoji is much easier.
As for topic icons, It probably would be better to implement using Emoji as icons as well.
Just thinking.

@marc1706 marc1706 modified the milestones: 3.2.0-RC3, 3.2.0-RC2 Nov 24, 2016

@marc1706 marc1706 modified the milestones: 3.2.1, 3.2.0-RC3 Jan 7, 2017

@marc1706 marc1706 modified the milestones: 3.2.2, 3.2.1 Jan 22, 2017

@CHItA

This comment has been minimized.

Copy link
Member

commented Sep 10, 2017

I'm closing this as it seems to be abandoned, feel free to reopen the PR if you wish to continue working on it.

@CHItA CHItA closed this Sep 10, 2017

@Leinad4Mind

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2018

This appears on changelog of 3.2.2 but wasnt added to the release?

@rxu

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2018

This wasn't actually added, just a changelog entry (by accident).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.