harmen/hypha
1
0
Fork 0
Code Issues 142 Pull requests 4 Projects Releases Packages Wiki Activity

Language selection #280

Merged
laurensmartina merged 14 commits from language-selection into master 2020-03-22 10:11:21 +00:00
Conversation 13 Commits 14 Files changed 13 +325 -107
laurensmartina commented 2019-12-16 14:25:50 +00:00 (Migrated from github.com)
Copy link
No description provided.
matthijskooijman (Migrated from github.com) reviewed 2019-12-16 14:25:50 +00:00
matthijskooijman (Migrated from github.com) requested changes 2020-01-20 10:39:42 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

Looks good to me. I left some minor inline remarks, of course :-)

Looks good to me. I left some minor inline remarks, of course :-)
index.php
@ -109,6 +107,10 @@
* @deprecated Use O_O instead
matthijskooijman (Migrated from github.com) commented 2020-01-20 10:27:38 +00:00
Copy link

Why not use __() here? The only caveat I can think of is that it would return _locale if not found, but that seems easy to check?

Why not use `__()` here? The only caveat I can think of is that it would return `_locale` if not found, but that seems easy to check?
system/core/language.php
@ -30,0 +20,4 @@
/**
* @return array Associative array containing connecting iso639 language codes with their native name (and english translation)
*/
public static function getIsoList() {
matthijskooijman (Migrated from github.com) commented 2020-01-20 10:13:58 +00:00
Copy link

This is probably a good moment to add html escaping, and remove this pointless addslashes (probably in a subsequent commit, though).

This is probably a good moment to add html escaping, and remove this pointless addslashes (probably in a subsequent commit, though).
system/core/pages.php
@ -107,0 +96,4 @@
<?php
$popup = <<<EOF
<table class="section">
<tr>
matthijskooijman (Migrated from github.com) commented 2020-01-20 10:20:42 +00:00
Copy link

Should this maybe preselect the default or current language? Now, nothing is marked as selected, so it just defaults to the first element in the list.

Should this maybe preselect the default or current language? Now, nothing is marked as selected, so it just defaults to the first element in the list.
system/core/pages.php
matthijskooijman (Migrated from github.com) commented 2020-01-20 10:23:04 +00:00
Copy link

I think this should validate the language code, since AFAICS hypha_addPage just uses it as-is.

I think this should validate the language code, since AFAICS `hypha_addPage` just uses it as-is.
matthijskooijman commented 2020-02-24 15:40:23 +00:00 (Migrated from github.com)
Copy link

@laurensmartina I see you pushed to this PR, but it seems my comments are still unfixed. Did you just rebase on top of master maybe?

@laurensmartina I see you pushed to this PR, but it seems my comments are still unfixed. Did you just rebase on top of master maybe?
laurensmartina (Migrated from github.com) reviewed 2020-03-02 16:57:15 +00:00
system/core/language.php
@ -30,0 +20,4 @@
/**
* @return array Associative array containing connecting iso639 language codes with their native name (and english translation)
*/
public static function getIsoList() {
laurensmartina (Migrated from github.com) commented 2020-03-02 16:57:14 +00:00
Copy link

Tested it, and removing the addslashes results in errors.

Tested it, and removing the `addslashes` results in errors.
laurensmartina (Migrated from github.com) reviewed 2020-03-02 17:04:38 +00:00
system/core/pages.php
@ -107,0 +96,4 @@
<?php
$popup = <<<EOF
<table class="section">
<tr>
laurensmartina (Migrated from github.com) commented 2020-03-02 17:04:38 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 17:15:51 +00:00
index.php
@ -109,6 +107,10 @@
* @deprecated Use O_O instead
laurensmartina (Migrated from github.com) commented 2020-03-02 17:15:51 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2020-03-02 17:22:09 +00:00
system/core/pages.php
laurensmartina (Migrated from github.com) commented 2020-03-02 17:22:09 +00:00
Copy link

fixed at hypha_addPage

fixed at `hypha_addPage `
matthijskooijman (Migrated from github.com) reviewed 2020-03-09 17:25:54 +00:00
system/core/language.php
@ -30,0 +20,4 @@
/**
* @return array Associative array containing connecting iso639 language codes with their native name (and english translation)
*/
public static function getIsoList() {
matthijskooijman (Migrated from github.com) commented 2020-03-09 17:25:53 +00:00
Copy link

I've removed the addSlashes and added htmlspecialchars, just force pushed this.

I've removed the addSlashes and added htmlspecialchars, just force pushed this.
matthijskooijman commented 2020-03-09 17:26:58 +00:00 (Migrated from github.com)
Copy link

I just rebased this on master and added a few more commits (one at the end, and a handful at the beginning). @laurensmartina, could you review my new commits? If they're ok, I think this is ready to be merged.

I just rebased this on master and added a few more commits (one at the end, and a handful at the beginning). @laurensmartina, could you review my new commits? If they're ok, I think this is ready to be merged.
laurensmartina (Migrated from github.com) reviewed 2020-03-22 08:08:53 +00:00
laurensmartina (Migrated from github.com) left a comment
Copy link

There is an inconsistency with the return type in the annotation and the actual return type. Please make it consistent.

There is an inconsistency with the return type in the annotation and the actual return type. Please make it consistent.
laurensmartina (Migrated from github.com) reviewed 2020-03-22 08:10:20 +00:00
system/php-dom-wrapper/Traits/ManipulationTrait.php
laurensmartina (Migrated from github.com) commented 2020-03-22 08:09:25 +00:00
Copy link

There is an inconsistency with the return type in the annotation and the actual return type. Please make it consistent.

There is an inconsistency with the return type in the annotation and the actual return type. Please make it consistent.
laurensmartina (Migrated from github.com) reviewed 2020-03-22 08:11:50 +00:00
laurensmartina (Migrated from github.com) left a comment
Copy link

The changes look good overal. The is just one little inconsistency

The changes look good overal. The is just one little inconsistency
matthijskooijman commented 2020-03-22 10:10:37 +00:00 (Migrated from github.com)
Copy link

Thanks for the review. I've fixed the comment, will merge this now.

Thanks for the review. I've fixed the comment, will merge this now.
Sign in to join this conversation.
Reviewers
No reviewers
matthijskooijman
Labels
Clear labels   
Component: User interface

   
Component: Wymeditor

   
Help wanted

   
Level: Difficult

Structural changes, need discussion

  
Level: Easy

Under 4 hours of work

  
Level: Moderate

Under 16 hours of work

  
Pagetype: Festival

   
Pagetype: Mailinglist

   
Pagetype: Peer reviewed article

   
Pagetype: Text

   
Privacy GDPR AVG

   
status: has conflicts

   
Status: Needs changes

   
Status: Needs discussion

   
Status: Needs review

   
Status: Ready to merge

   
Status: Waiting for response

   
Type: Bug

   
Type: Enhancement

   
Type: Question

   
Usecase: De Stadsbron

   
Usecase: Koppelting

   
Usecase: MeetjeStad

   
Value: Coders

Enhancement of codebase, API, documentation

  
Value: Security

   
Value: Users

Improve user experience, add new features

  
Value: Visitors

Improve navigation, styling

No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
harmen/hypha!280
No description provided.