-
-
Notifications
You must be signed in to change notification settings - Fork 78
Add devblog #179
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
Add devblog #179
Conversation
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.
Fantastic PR - thanks @gwleuverink!
Just a few requested changes and then I think we'll be g2g:
- auto-slug after inputting a
title
- allowing admins to view an unpublished article
- update article styling for a few element types
@steven-fox Your feedback made me reconsider the scheduling input. Having it null by default still shows a date + considering the preview feature it seems it's more in the way than helpful. I might remove the input altogether and see if I can do this with actions instead. |
Hmm. If I modify the Sure, if I click that calendar icon, it shows today's date & time within the calendar, but that's not a big concern and just an artifact of the HTML5 date picker component. I think it'll mainly be Simon and Shane writing articles, so we don't have to go crazy with a UX for non-technical users. 😄 |
Something I didn't bring up in my review (because I think it's kinda overkill right now) is the fact that updating a slug on a published article could lead to dead links. An easy way to get around this is to define the route as @simonhamp Do you have a strong opinion on this right now? Do you want to handle changing slugs and routes now or leave the route defined as |
I'm down to add this now rather than later. Would save me a redirect script down the line. Just a personal nit-pick; I don't like id's in public facing url's. If it's okay I can bring in Hashids to obfuscate that |
Let's see if Simon has anything to say. I believe he's going to lean more towards the "keep it super simple, we won't alter slugs" approach and I'm also in that camp right now. I didn't intend for this PR to grow in complexity haha. Besides, we can always roll with the setup you implemented now, and if the slug stuff becomes an issue, solve it in a way that supports slug revisions and the old urls. At the end of the day Willem, the article styling is the ONLY piece of my requested changes that is absolutely necessary before merging the PR as-is. Everything else we're talking about here is an enhancement that can come in future, tiny PR's if/when it's desired by those writing articles. Why don't we take care of that styling, merge this PR, and then Simon/Shane can be aware of this convo for future tasks. 👍 |
Let's just not change slugs 👍 |
Gotcha 👍🏻
|
Couldn't help myself and went ahead and added:
Let me know if anything needs adjustment 🙌🏻 |
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 think this turned out great! Awesome work. 👍
Glued the existing blog templates to the back-end.
I added some guards for scheduling posts in the future. Should be handy. Wrote some tests around that too.
Lmk if I need to change anything 👍🏻