-
Notifications
You must be signed in to change notification settings - Fork 19
Storybook updates #148
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
Storybook updates #148
Conversation
docs/database-schema.md
Outdated
# Database Schema | ||
|
||
### wp_notifications table | ||
* id (int) 11 auto_increment `ID of the notification.` | ||
* sender_key `Sender of the notification. As defined by the plugin or theme` | ||
* timestamp `Timestamp of when the notification was triggered.` | ||
* recipient_key `Single notification recipient. As defined by the plugin or theme` | ||
* title_key `Notification title key. As defined by the plugin or theme` | ||
* message_key `Notification message key. As defined by the plugin or theme` (optional) | ||
* action_link `Correctly formatted URL to an internal or external action.` | ||
* status enum (READ, UNREAD) `Notification status.` | ||
|
||
See the Translations document for details on title_key and message_key fields |
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.
i'm reverting that changes... it's due a failed merge! sorry
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.
LGTM
^^ updating deps wasn't something that I didn't want to do in this pr, but I was forced |
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.
This is great, it's awesome to have such nice docs!
TODO: after creating the master branch we need to automate this documentation build process with the ci and run it at every merge between development and master |
.gitignore
Outdated
@@ -3,5 +3,6 @@ | |||
|
|||
/node_modules/ | |||
/vendor/ | |||
|
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.
@erikyo did you mean to add a line here?
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.
no... however it goes, let's say a collateral damage of the lint. I would leave it since, theoretically, the empty end line is required at the end of each document
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.
Makes sense to me, I agree empty lines between groups is standard.
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.
but you are right maybe a comment would be nice to separate things (for example, I hadn't noticed that he had a line underneath)
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.
I've started to make some updates to this in #240
@erikyo I don't like the idea of all the storybook build artifacts being checked into the repo. Is the |
Can we add a CI workflow for publishing to GitHub pages? |
I have already created an issue for this, however I would do it in a separate pr. if you think we'll take care of it shortly I'll remove what we have now in the docs folder, otherwise maybe we'd better leave it there |
I'll help you resolve it now. If we check those in, I think we will regret it quickly. |
This is the CI workflow used alongside the Gutenberg repository Seems quite easy to add, what o you think @johnhooks? |
@erikyo yes that looks perfect, but lets do it in a separate PR. Getting storybook functional is a very different issue than deploying to GitHub Pages. We will also probably need @Sephsekla to enable using GitHub Actions to build GitHub Pages in the settings tab of the repo. |
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.
LGTM
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.
There were no changes since the last approval, only rebased onto develop
. LGTM
@Sephsekla we need to merge this one after #252 in order to update this one with the new drawer before updating the whole repo. Please take care of that one first, both of these pr's are IMHO a big and nice advancement, sorry for the number of commits/files changed but since those are structural changes doing them in various PR's would have stalled development considerably |
… facilitate the ci action
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.
Nice!
Added one suggestion around naming, it doesn't necessarily have to be a blocker at this stage though.
.storybook/wp-notify-theme.js
Outdated
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.
Only just noticed this, but could we update the naming here to reference WP Feature Notifications instead of WP Notify? Similar to what @johnhooks has been doing in PHP.
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.
Many of these are already updated in the storybook-wip branch, my only doubt was about css custom props because wp-notification-feature
is really too long to be used on this purpose! @Sephsekla @johnhooks probably because of these cases the "wp-notifications" prefix might work better? What do you think?
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.
I support this wp-feature-notifications
is too long of a prefix, or even adopting something that is shorter like the performance plugin did:
I don't think we want to change the rest api to... do we?
/wp-json/wp-feature-notifications/v1/notifications
I can't think of anything that rolls off the tounge as well as perflab
though... featnote
sounds awful.
Oops, hadn't noticed that this now has a couple of conflicts from the drawer update. As soon as those are resolved,re-request review and I can approve. |
updates to storybook 7.0.4
We have branched this PR to keep things clean, we are working on... and we are close to get it fully working! In addition there are some improvements over the previous version, such as the default open drawer (cc @johnhooks) and some useful modules as the one that performs a a11y checking automatically for each component. |
@johnhooks, also we might want to provide in this PR the workflow for publishing the storybook? An idea of what it needs to look like is this one (and I got it here) |
Setup node and install deps. I think it would work if you move into the - name: Setup Node.js and install dependencies
uses: ./.github/setup-node Scratch that one above, I didn't realized that second one you linked is the Gutenberg repo. There are some changes I would suggest, can you add it to this PR and I'll make some inline comments? |
@johnhooks since i am not sure, I would prefer if you could add the workflow part to this PR, thanks! (I believe you have all the permissions to do that, but if not tell me) |
…e the first notice of the context "adminbar")
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.
LGTM 🤘🏻
Apologies for the late response as I was catching up from vacation! Glad you all got it sorted 👍 |
What?
#147 Add an autogenerated documentation to Github pages
Why?
Actually we haven't a (easy to reach) user documentation.
How?
Thanks to the capability of storybook to generate a static website we have the possibility to see the components in action and have a mini-site to show the documentation (while still using md files, if we prefer, as I did here)
close #147, and closes #241
I had to synchronize the branch with the master, which also forced me to update some dependencies. Little harm either way, maybe we get fewer alerts from dependbot