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

Various fixes and enhancements #889

Closed
wants to merge 12 commits into from
Closed

Various fixes and enhancements #889

wants to merge 12 commits into from

Conversation

niol
Copy link
Collaborator

@niol niol commented Mar 14, 2017

Details are in the individual patches.

This should fix fossar#756 and give a workaround for other cookie related issues.
* @param message string
*/
showError: function(message) {
selfoss.ui.showMessage(message, 'undefined', 'undefined', true);
Copy link
Member

@jtojnar jtojnar Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be quote-less, since typeof 'undefined' === 'string', leading to showing button with undefined text in error messages (e.g. invalid fragment like #invalid)

@jtojnar
Copy link
Member

jtojnar commented Mar 14, 2017

I am not sure if #879 should be fixed right now, since it is a BC break. I will ask Tobias about BC break policy.

@jtojnar
Copy link
Member

jtojnar commented Mar 14, 2017

For now, it would be probably better to leave out the commit since we are essentially on 2.17 feature freeze.

@jtojnar
Copy link
Member

jtojnar commented Mar 14, 2017

There is still some work to do, for example, tags key in /items endpoint should be an array of tags; separate pull request will probably be for the best.

@jtojnar
Copy link
Member

jtojnar commented Mar 14, 2017

Another issue with the async refresh: refreshing on the sources page will keep loading forever with the error selfoss.lastUpdate is null in the console.

@jtojnar
Copy link
Member

jtojnar commented Mar 14, 2017

Another point against the API change: 4eb1891 breaks adding sources.

@@ -165,7 +165,7 @@ public function write() {
unset($data['ajax']);

// check if source already exists
$id = \F3::get('PARAMS["id"]');
$id = intval(\F3::get('PARAMS["id"]'));
Copy link
Member

@jtojnar jtojnar Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the source adding since intval('new-123') === 0, which is a valid ID according to $sourcesDao->isValid.

@@ -144,13 +144,15 @@ public function get($id = null) {
// select source by id if specified or return all sources
if (isset($id)) {
$ret = \F3::get('db')->exec('SELECT id, title, tags, spout, params, filter, error FROM ' . \F3::get('db_prefix') . 'sources WHERE id=:id', [':id' => $id]);
$this->stmt->ensureRowTypes($ret, ['id' => \PDO::PARAM_INT]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be assigned to $ret.

if (isset($ret[0])) {
$ret = $ret[0];
} else {
$ret = false;
}
} else {
$ret = \F3::get('db')->exec('SELECT id, title, tags, spout, params, filter, error FROM ' . \F3::get('db_prefix') . 'sources ORDER BY error DESC, lower(title) ASC');
$this->stmt->ensureRowTypes($ret, ['id' => \PDO::PARAM_INT]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be assigned to $ret.

#message a {
font-size:0.9em;
text-align:center;
-moz-border-radius:2px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for prefixed versions, since border-radius is widely supported.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prefixed border-radius was removed everywhere but here.

@jtojnar
Copy link
Member

jtojnar commented Mar 14, 2017

Additionally, 00ced63 broke error handling when adding source. Try adding http://example.com as an RSS feed with no title: the server will correctly return error but client will keep loading.

@@ -261,7 +264,7 @@ var selfoss = {
since: selfoss.lastUpdate.toISOString(),
tags: true,
sources: selfoss.filter.sourcesNav,
items_statuses: true
items_statuses: selfoss.lastUpdate !== null
Copy link
Member

@jtojnar jtojnar Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won’t this now always be true, since there is line 256?

@jtojnar
Copy link
Member

jtojnar commented Mar 15, 2017

Rebased and merged as 6beba94

@jtojnar jtojnar closed this Mar 15, 2017
jtojnar added a commit that referenced this pull request Nov 27, 2017
The overflow was initially enabled for preformatted text and blockquotes
in 77b09ef but it was somewhat quizzically
reverted in #674 and then reintroduced for <pre> in #889.

This time we are enabling it for the tables and removing the forgotten
blockquote code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants