Feature/notification reworked design #66

erikyo · 2022-03-08T14:31:34Z

The design has been updated
Adds webpack

Folder structure:
includes/ui/notification-hub/assets - The work folder
assets/html - here you will find the notices and the sidebar html
assets/script - here for now you will find some scripts that has only the function to test the ui
assets/css/notify.scss - the WP-Notify style (this plugin css)
assets/css/wordpress.scss - The WordPress Admin style (will not be included in the future, but at the moment is needed for ui testing purpose)
img/ - actually there is the favicon (without that there is an error in console)
includes/ui/notification-hub/dist - Build folder

- to make sure that the pages are seen with gh pages I moved dist to docs - cleanup

jonathanbossenger · 2022-03-08T14:40:30Z

Thanks @erikyo I will work on reviewing it over the course of this week.

.browserslistrc

-renamed settings.html to settings-content.html because i've got some issue with duplicated filenames

.gitignore

jonathanbossenger · 2022-03-23T14:25:00Z

@Sephsekla from my POV this looks good. If you have time to look this over, please do.

Alternatively, if you don't have time for a full review, it would be helpful to compare what you've done to this PR, and confirm that if we merge this, it compliments your work. From what I can see the folder structure is slightly different, in that you've built your work on the notification-hub directrory, and this PR builds everything in the docs directory.

I just want to make sure we've not lost anything important you included in your branch.

jonathanbossenger · 2022-05-04T14:28:11Z

@erikyo thanks for your patience here, it's been a busy few months.

So, I have a few things I'd like to see if we can get implemented into this PR, in order to make it more easily "testable" by end users.

Currently for the user to view this, they will need to know to navigate to /wp-content/plugins/wp-notify/docs/index.html. I understand why this is, but I'd like to make it easier for someone to install the plugin and find the designs. Would you be able to register a custom Admin menu (maybe something like WP Notify) that would direct the user to this URL instead?
At the moment the "Try this new feature" buttons slide open the Notifications hub. Could we rather change that so that it opens the link to this repository https://github.com/WordPress/wp-notify in a new tab?
The HTML title reads <title>WordPress Dashboard + <%= htmlWebpackPlugin.options.title %></title> could we fix that so that instead of <%= htmlWebpackPlugin.options.title %> it says WP Notify?

If we could make those changes, it makes it easier to share this as an installable plugin, which users can install and test.

Let me know if you need any help with any of these items.

erikyo · 2022-05-10T12:15:11Z

All clear but I just have one small question:

Would you be able to register a custom Admin menu (maybe something like WP Notify) that would direct the user to this URL instead?

Where i have to register the custom Admin menu in the wp-notify plugin (here?). Let me know!

jonathanbossenger · 2022-05-11T14:41:52Z

@erikyo for now, I suggest we just add the admin menu procedurally to the main wp-notify.php file. This admin menu will be removed when we actually develop the notifications functionality, it's just to allow folks to install and see this in action, and hopefully generate some interest.

Sephsekla

Thanks for all the hard work here, this has really got things moving again!

I haven't yet tested it out, so just a general scan of the code, but I've added a few comments and suggestions. Some are really nit-picky, but I've just added things that have come to mind as I've read through.

README.md

includes/ui/notification-hub/assets/script/script.js

Sephsekla · 2022-05-13T14:40:48Z

includes/ui/notification-hub/assets/script/script.js

+    let title = document.getElementById('title').value;
+    let content = document.getElementById('content').value;
+
+    var div  = document.createElement('div');


Ditto this can be a const, since we're just mutating properties, rather than ever setting it again.

Sephsekla · 2022-05-13T14:47:31Z

includes/ui/notification-hub/assets/style/notify/bullet.scss

+
+#wpadminbar .wp-notification.unread >:first-child:before {
+  top: calc(50% - #{$bullet-width * .5 });
+  left: #{ -24px * .5 - $bullet-width * .5 }; // 9 - left padding - border /2  + bullet/2


These seem really fiddly, I'm wondering if there's a clearer way to handle the variables etc here.

I honestly don't know how to solve it here, but it can be simplified with #{ (-24px - $bullet-width) * .5 }

includes/ui/notification-hub/assets/style/notify/notification-hub.scss

Sephsekla · 2022-05-13T14:50:32Z

includes/ui/notification-hub/assets/style/notify/notify.scss

+/*
+ * General notifications style
+ */
+.wp-notification {


I'm conscious we have styles for the individual notifications in both this file and the hub file - not necessarily a bad thing but I do wonder if it could be slightly unclear for future maintenance.

I removed from here all the css properties apart from colors and lettering (which are indeed shared between the two types of notifications), tell me if this is an acceptable solution for you.

Sephsekla · 2022-05-13T14:57:25Z

webpack.config.js

@@ -0,0 +1,188 @@
+const isProduction = process.env.NODE_ENV === 'production';


I'd maybe split this file out and import a couple of things, but that's a matter of personal preference more than anything.

here the issue was solved because webpack is no longer used as long we don't need no more the dev server 😉... checkout at this branch where I am integrating this style (with all your suggestions)

Sephsekla · 2022-05-13T15:01:00Z

includes/ui/notification-hub/assets/html/notices.html

@@ -0,0 +1,50 @@
+<div class="wp-notification wp-notice-alert is-dismissible">


Given you've already got the html <include> setup for use elsewhere, what do you think about splitting these out into smaller files in a subfolder and including them? Just for the purpose of keeping things as modular as possible and easy to see what's what.

Feature/notification Design implementation (follow-up of #66)

jonathanbossenger · 2022-06-02T09:45:50Z

Superceeded by #67

erikyo and others added 5 commits March 6, 2022 14:37

mockup update

564169d

chore

d7e7c9b

- to make sure that the pages are seen with gh pages I moved dist to docs - cleanup

initial desktop settings commit

d6cc7e9

adds text settings link

fd339e3

adds basic notice dismissal

a590780

jonathanbossenger self-requested a review March 8, 2022 14:40

Sephsekla reviewed Mar 9, 2022

View reviewed changes

.browserslistrc Outdated Show resolved Hide resolved

erikyo added 5 commits March 9, 2022 14:38

close WordPress#66 (comment)

82abcc9

link to settings page

1b1f476

better template splitting

c69a874

-renamed settings.html to settings-content.html because i've got some issue with duplicated filenames

Utility function to add a new notification using the "Quick draft" form

a8ea1c5

removes erik customizations

9ca86bb

Sephsekla reviewed Mar 14, 2022

View reviewed changes

.gitignore Show resolved Hide resolved

erikyo added 2 commits March 14, 2022 13:15

prevents random mac files from being committed

0e12b2e

cherry-picked css grid update + build

9c6e645

Sephsekla reviewed May 13, 2022

View reviewed changes

erikyo mentioned this pull request May 18, 2022

Feature/notification Design implementation (follow-up of #66) #67

Merged

erikyo closed this May 21, 2022

jonathanbossenger reopened this Jun 1, 2022

jonathanbossenger added a commit that referenced this pull request Jun 1, 2022

Merge pull request #67 from erikyo/design_implementation

6c4de07

Feature/notification Design implementation (follow-up of #66)

jonathanbossenger closed this Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/notification reworked design #66

Feature/notification reworked design #66

erikyo commented Mar 8, 2022

jonathanbossenger commented Mar 8, 2022

jonathanbossenger commented Mar 23, 2022

jonathanbossenger commented May 4, 2022

erikyo commented May 10, 2022

jonathanbossenger commented May 11, 2022

Sephsekla left a comment

Sephsekla May 13, 2022

Sephsekla May 13, 2022

erikyo May 15, 2022

Sephsekla May 13, 2022

erikyo May 15, 2022

Sephsekla May 13, 2022

erikyo May 15, 2022

Sephsekla May 13, 2022 •

edited

Loading

jonathanbossenger commented Jun 2, 2022

		@@ -0,0 +1,188 @@
		const isProduction = process.env.NODE_ENV === 'production';

		@@ -0,0 +1,50 @@
		<div class="wp-notification wp-notice-alert is-dismissible">

Feature/notification reworked design #66

Feature/notification reworked design #66

Conversation

erikyo commented Mar 8, 2022

jonathanbossenger commented Mar 8, 2022

jonathanbossenger commented Mar 23, 2022

jonathanbossenger commented May 4, 2022

erikyo commented May 10, 2022

jonathanbossenger commented May 11, 2022

Sephsekla left a comment

Choose a reason for hiding this comment

Sephsekla May 13, 2022

Choose a reason for hiding this comment

Sephsekla May 13, 2022

Choose a reason for hiding this comment

erikyo May 15, 2022

Choose a reason for hiding this comment

Sephsekla May 13, 2022

Choose a reason for hiding this comment

erikyo May 15, 2022

Choose a reason for hiding this comment

Sephsekla May 13, 2022

Choose a reason for hiding this comment

erikyo May 15, 2022

Choose a reason for hiding this comment

Sephsekla May 13, 2022 • edited Loading

Choose a reason for hiding this comment

jonathanbossenger commented Jun 2, 2022

Sephsekla May 13, 2022 •

edited

Loading