(feat): wordpress menu based sidenav instead of hardcoded sidenav. also added all den haag nlds icons #30
Conversation
…so added all den haag nlds icons
| */ | ||
| public static function updateNavItems( $menu_id, $menu_item_db_id ) { | ||
| foreach ( self::getFields() as $field_name => $meta_data ) { | ||
| $value = ( |
There was a problem hiding this comment.
Je zou deze in 1 regel kunnen schrijven eventueel:
$value = ($_POST[ $field_name ][ $menu_item_db_id ] ?? '') == 'on';
There was a problem hiding this comment.
Fair enough! Morgen pas ik de boel even aan, dan sta ik weer ingepland.
| * | ||
| * @return array[] | ||
| */ | ||
| public static function getFields() { |
There was a problem hiding this comment.
Mierenneuken:
public static function getFields(): array
{
//
}
There was a problem hiding this comment.
Dan ga ik er van uit dat we in dit project alle functies strict typing maken? (Even voor de zekerheid)
There was a problem hiding this comment.
Wat mij betreft wel! Echter ben ik maar 1 persoon natuurlijk.
| * | ||
| * @return void | ||
| */ | ||
| public static function addCustomFields( $item_id, $item ) { |
There was a problem hiding this comment.
Niet persé fout maar waarom zijn de methods static?
There was a problem hiding this comment.
Eerlijke antwoord is gewenning/preference van schrijven. Zelf vind ik class::function() mooier staan dan class->function(), in ieder geval op plaatsen waar het eigenlijk vrij weinig uit maakt zoals hier.
Ik probeer echter wel de stijl van het project zo veel mogelijk over te nemen dus als je liever non-static functies hebt dan pas ik dat uiteraard aan! :-)
There was a problem hiding this comment.
In deze context werkt dit prima alleen strookt het met datgeen wat ik geleerd heb. Wellicht dat deze klasse ooit uitgebreid wordt waardoor je de static methods alsnog moet vervangen. Stukje leesvoer voor de liefhebber.
YvetteNikolov
force-pushed
the
mijn-zaken/frontend
branch
from
e3d115c to
6611ca7
Compare
robertbossaert
force-pushed
the
mijn-zaken/frontend
branch
from
6611ca7 to
2d4662a
Compare
YvetteNikolov
force-pushed
the
mijn-zaken/frontend
branch
from
8a7db0c to
4d040df
Compare
robertbossaert
force-pushed
the
mijn-zaken/frontend
branch
from
c8d3d8f to
03c6778
Compare
|
@mvdhoek1 - mogguh, zou jij nog een review kunnen geven van de changes? de merge conflict wordt steeds groter 😅 |
|
@Jikizuari heb je ge-rebased? Had begrepen dat jullie dat niet vaak gebruiken. Ik kan wel even meekijken als je dat fijn vindt. |
|
@mvdhoek1 Ik rebase eigenlijk nooit. Mocht je het magisch kunnen oplossen zal dat 100% sneller zijn dan de methode die ik zou gebruiken :-P |
|
@Jikizuari ik heb een poging gedaan maar ik weet niet precies welke delen van jou zijn en nieuw zijn en wat niet. Ik vraag Yvette even omdat zij er meer in zit. |
mvdhoek1
force-pushed
the
mijn-zaken/frontend
branch
3 times, most recently
from
5c9e055 to
412bd7e
Compare
|
Hier moeten we nog even naar gaan kijken. |
mvdhoek1
force-pushed
the
mijn-zaken/frontend
branch
5 times, most recently
from
3c9841c to
7c6a0ab
Compare
mvdhoek1
force-pushed
the
mijn-zaken/frontend
branch
from
fdc044c to
a726a42
Compare
YvetteNikolov
force-pushed
the
mijn-zaken/frontend
branch
from
7d3e9d8 to
09105dc
Compare
mvdhoek1
force-pushed
the
mijn-zaken/frontend
branch
from
09105dc to
6b5b9ef
Compare
nieuwe branch gemaakt, ik ga de oude ook verwijderen.