-
Notifications
You must be signed in to change notification settings - Fork 0
Dev #1
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
Dev #1
Conversation
Composer package changes
|
|
Coverage report for commit: c3bc844 Summary - Lines: - | Methods: -
🤖 comment via lucassabreu/comment-coverage-clover |
||||||||||||||||||||||||||||||||
| ->name('components') | ||
| ->hasConfigFile() | ||
| ->hasViews('brave') | ||
| ->hasViewComponent('brave', PatternContent::class) |
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.
hasViewComponents('brave', [PatternContent::class, BackButton::class]
laravdiemen
left a comment
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.
Ziet er vet uit! De inhoudelijke review laat ik aan backend over ;)
|
|
||
| class BackButton extends Component | ||
| { | ||
| public string $link = 'javascript:history.back();'; |
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.
Deze kunnen toch gewoon naar de constructor?
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.
Nee, kreeg de error Typed property App\View\Components\BackButton:: must not be accessed before initialization.
Het gebeurde (volgens mij) als ik een terugknop blok op een pagina zetten, en dat hij geen parent page had.
Simon heeft hier een fix doorgevoerd https://github.com/yardinternet/brave/pull/121/files en heb die file gekopieerd (+ verbeterd om PHPStan tevreden te stellen)
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.
Weird, ik zou denken dat ze met een default waarde wel voldoende geinitialiseerd waren
|
Meer packages = meer beter 🥳 |
src/Hooks/PatternContent.php
Outdated
| $patternConfig = $this->patterns[$post['post_name']] ?? null; | ||
|
|
||
| if ($patternConfig && ! empty($patternConfig['save_as_draft'])) { | ||
| $data['post_status'] = 'draft'; | ||
| } |
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.
How about:
| $patternConfig = $this->patterns[$post['post_name']] ?? null; | |
| if ($patternConfig && ! empty($patternConfig['save_as_draft'])) { | |
| $data['post_status'] = 'draft'; | |
| } | |
| $patternConfig = $this->patterns[$post['post_name']] ?? []; | |
| if ($patternConfig['save_as_draft'] ?? false ) { | |
| $data['post_status'] = 'draft'; | |
| } |
Zo ben je meteen die sloppy empty() kwijt en is je type safety stukken beter.
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.
Beter!
Brave components package
In overleg met Simon deze components package gecreëerd. We merkten dat best wat logica-heavy componenten gekopieerd worden van project naar project, zoals de Back Button. Met deze package is het beter te maintainen
Back Button
Zoals jullie die kennen. Er komt een PR in Brave aan van dit:
naar dit
Pattern Content (nieuw)
Met dit component kun je gemakkelijk gesynchroniseerde patroon content ophalen en tonen op basis van de slug. Dit gebruiken we zodat klanten stukken kan bewerken die normaal in de code staan. Zoals de footer, of een info blok op een single vacature die buiten de content staat.
Voorbeeld:
Daarnaast kun je via de config instellen of een patroon in de admin vergrendeld moet worden en een custom label moet krijgen. Default:
En wat het doet:
