[SDPAP-6839] Add Dismissible Message Bar for CMS user #362
Conversation
vincent-gao
force-pushed
the
feature/SDPAP-6839-Dismissible-Message-Bar-for-CMS-user
branch
2 times, most recently
from
f37d5f2
to
dda4d45
Compare
vincent-gao
changed the title
vincent-gao
force-pushed
the
feature/SDPAP-6839-Dismissible-Message-Bar-for-CMS-user
branch
from
a9affd7
to
f5cef80
Compare
update to override the parent css lint Updates 1. Update label options 2. hide severity field 3. set severity to medium 4. css update 5. twig update 6. install the site alert block Put message next to the label update site alert theme fixed the overflow issue lint
vincent-gao
force-pushed
the
feature/SDPAP-6839-Dismissible-Message-Bar-for-CMS-user
branch
from
f5cef80
to
47a2353
Compare
vincent-gao
requested review from
MdNadimHossain,
edyuenyw,
anthony-malkoun and
krakerag
|
wow it is already here, nice one @vincent-gao |
There was a problem hiding this comment.
LGTM @vincent-gao. A couple of code suggestions but it is fine the way it is.
| if ($value && strpos($value, 'site_alert:') !== FALSE) { | ||
| preg_match('#[0-9]+$#', $value, $match); | ||
| $site_alert = SiteAlert::load(reset($match)); | ||
| $build[$key]['#alert']['start'] = date('d/m/Y H:i:s', strtotime($site_alert->getStartTime())); |
There was a problem hiding this comment.
I'm a bit of a stickler for DateTimeImmutable since you can trust the time you set on it hasn't changed.
There was a problem hiding this comment.
Hey @krakerag , thanks for the CR.
It is always good to learn new things!
I have made a further change trying to use DateTimeImmutable class. please take a look.
| * Implements hook_form_alter(). | ||
| */ | ||
| function tide_site_alert_form_alter(&$form, FormStateInterface $form_state, $form_id) { | ||
| if ($form_id === 'site_alert_add_form' || $form_id === 'site_alert_edit_form') { |
There was a problem hiding this comment.
Usually better to refactory your if statements so you can return immediately and then have the code nested lower. It's a bit of an anti-pattern to have your entire function nested in an if statement
There was a problem hiding this comment.
This reminds me that I should switch to using another hook!
| */ | ||
| function tide_site_alert_textfield_callback(&$form, FormStateInterface $form_state) { | ||
| $selected = $form_state->getValue('suggested_labels'); | ||
| if ($selected != 'Other') { |
There was a problem hiding this comment.
I like to set something and then only have the if statement. Elses can normally be removed when it's just setting one value or another.
Thanks for the CR @MdNadimHossain |
| @@ -29,8 +29,11 @@ public function build() { | |||
| if ($value && strpos($value, 'site_alert:') !== FALSE) { | |||
| preg_match('#[0-9]+$#', $value, $match); | |||
| $site_alert = SiteAlert::load(reset($match)); | |||
| $build[$key]['#alert']['start'] = date('d/m/Y H:i:s', strtotime($site_alert->getStartTime())); | |||
| $build[$key]['#alert']['end'] = date('d/m/Y H:i:s', strtotime($site_alert->getEndTime())); | |||
| $dateTimeImmutable = new \DateTimeImmutable(); | |||
There was a problem hiding this comment.
Hmm I think this might not be correct.
DateTimeImmutable should be replacing date() - you're creating a new DateTimeImmutable object to work with, to create a formatted output for your two build variables. It should probably be more like this:
$build[$key]['#alert']['start'] = new \DateTimeImmutable($site_alert->getStartTime())->format('d/m/Y H:i:s');
$build[$key]['#alert']['end'] = new \DateTimeImmutable($site_alert->getEndTime())->format('d/m/Y H:i:s');
With your code, you're creating a new object based on time now(), and then modifying it by your start times/end times.
Does this make sense?
vincent-gao
deleted the
feature/SDPAP-6839-Dismissible-Message-Bar-for-CMS-user
branch
Jira
https://digital-vic.atlassian.net/browse/SDPAP-6400
Problem/Motivation
There is currently no way for users to know if a CMS is undergoing maintenance or has upcoming maintenance or content freeze.
Related PRs
https://github.com/dpc-sdp/content-vic-gov-au/pull/1455
Screenshots
TODO
The current suggested labels are hard coded, it may be worth providing a form for admin users in the near future...