-
-
Notifications
You must be signed in to change notification settings - Fork 776
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
New: Sharing articles from the article title line #6395
base: edge
Are you sure you want to change the base?
Conversation
The reading view is untouched yet. It will prevent conflicts with #6297. Sharing will be added later. |
I like the idea, but I would like to suggest a more powerful solution: make a menu from the title line as you have, but allowing multiple functions, the sharing one being a sub-menu. |
$id = $this->entry->id(); | ||
$link = $this->entry->link(); | ||
$title = $this->entry->title() . ' · ' . ($this->feed === null ? '' : $this->feed->name()); | ||
foreach (FreshRSS_Context::userConf()->sharing as $share_options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation consideration: this adds a lot of data in the output (consume resources server side, at network level, and client side) for a non-essential shortcut feature. To save resources and improve performances, I think this should be left to a dynamically-generated menu like we have for the configuration drop-down menu of the feeds in the aside bar.
Example:
FreshRSS/app/layout/aside_feed.phtml
Lines 153 to 189 in 329fd4b
<script id="tag_config_template" type="text/html"> | |
<ul class="dropdown-menu"> | |
<li class="item"> | |
<a class="configure open-slider" href="<?= _url('tag', 'update', 'id', '------') ?>"><?= _t('gen.action.manage') ?></a> | |
</li> | |
</ul> | |
<a class="dropdown-close" href="#close">❌</a> | |
</script> | |
<script id="feed_config_template" type="text/html"> | |
<ul class="dropdown-menu"> | |
<li class="item"><a href="<?= _url('index', $actual_view, 'get', 'f_------') ?>"><?= _t('gen.action.filter') ?></a></li> | |
<?php if (FreshRSS_Auth::hasAccess()) { ?> | |
<li class="item"><a href="<?= _url('stats', 'repartition', 'id', '------') ?>"><?= _t('index.menu.stats') ?></a></li> | |
<?php } ?> | |
<li class="item link website"><a target="_blank" rel="noreferrer" href="http://example.net/"><?= _t('gen.action.see_website') ?></a></li> | |
<?php if (FreshRSS_Auth::hasAccess()) { | |
$get = Minz_Request::paramString('get'); | |
if ($get === '') { | |
$url = _url('subscription', 'feed', 'id', '------', 'from', $actual_view); | |
} else { | |
$url = _url('subscription', 'feed', 'id', '------', 'get', $get, 'from', $actual_view); | |
} | |
?> | |
<li class="item"><a class="configure open-slider" href="<?= $url ?>"><?= _t('gen.action.manage') ?></a></li> | |
<li class="item"><a href="<?= _url('feed', 'actualize', 'id', '------') ?>"><?= _t('gen.action.actualize') ?></a></li> | |
<li class="item"> | |
<?php $confirm = FreshRSS_Context::userConf()->reading_confirm ? 'confirm" disabled="disabled' : ''; ?> | |
<button class="read_all as-link <?= $confirm ?>" | |
form="mark-read-aside" | |
formaction="<?= _url('entry', 'read', 'get', 'f_------') ?>" | |
type="submit"><?= _t('index.menu.mark_feed_read') ?></button> | |
</li> | |
<?php } ?> | |
</ul> | |
<a class="dropdown-close" href="#close">❌</a> | |
</script> |
Or (probably even better) use an HTML template element https://developer.mozilla.org/docs/Web/HTML/Element/template like:
FreshRSS/app/views/configure/integration.phtml
Lines 15 to 31 in 329fd4b
<template id="simple-share"> | |
<fieldset class="group-share dragbox"> | |
<legend draggable="true">##label##</legend> | |
<input type="hidden" id="share_##key##_type" name="share[##key##][type]" value="##type##" data-leave-validation="" /> | |
<div class="form-group" id="group-share-##key##"> | |
<label class="group-name" for="share_##key##_name"><?= _t('conf.sharing.share_name') ?></label> | |
<div class="group-controls"> | |
<input type="text" id="share_##key##_name" name="share[##key##][name]" value="##label##" /> | |
</div> | |
</div> | |
<div class="form-group"> | |
<div class="group-controls"> | |
<button type="button" class="remove btn btn-attention" title="<?= _t('conf.sharing.remove') ?>"><?= _t('gen.action.remove') ?></button> | |
</div> | |
</div> | |
</fieldset> | |
</template> |
@Alkarex Thanks for your feedback. Let me explain my roadmap that I have in mind: TODO: I was thinking to use the templates too, because this menu is created in each article (also before this PR). I wanted to do this refactoring after this PR to keep focus on this feature and do not mix it with the refactoring of the existing sharing menu. The submenu thing: yes, I am thinking about it too, but there is some more brain power needed ;) Right now the code does not have any sub menu features. A sub menu needs one more click/interaction than just a button (I can remember some discussion about having 1 more click to more-or-less important features). I think it will be important to have this sub menu feature and more features as you described. We could play here with CSS container queries too, so that it is usable on small and big screens. My roadmap in bullet points to summarize it:
While doing it we should not forget to have a good integration into the reading view too. I am bit excited to implement it :) |
It adds a column for sharing articles directly in the normal view article row
config via display config (default: not selected)
Pull request checklist: