Announcements into a class

[R4z0rBl8D3]

this is my first pull request ever :)

I turned the post file in the server into a class and added a method to get a list of all the announcements.
And I made a test thing to show all the announcements.

and i added package-lock.json to the .gitignore because i used npm by accident

[R4z0rBl8D3]

Also, should I keep doing pull requests or make a separate branch or something? I've always just committed directly to my own repos so this is new to me

[R4z0rBl8D3]

just a note, i am definitely messing the typescript parameter stuff up because I have avoided typescript before this 😭

[R4z0rBl8D3]

also i found out that you can set custom CSS in directus :)))))))

.modules {
    background: background: rgb(255,223,0); background: linear-gradient(255deg, rgba(255,223,0,1) 0%, rgba(194,2,25,1) 100%); !important
}

[raisch]

All in all, a VERY SOLID first PR! Congrats. I've seen far less from devs with a lot more experience.

There are some changes I think it needs, so I'm going to leave this as "Request Changes", but... it's completely up to you whether you want to follow my advice.

If not, I'm happy to "Approve".

[raisch]

not needed. please remove.

[raisch]

probably a good idea to surface the error message as well so we can triage any problems

[raisch]

why not use an :else here?

[raisch]

One good rule of thumb for templates such as these is you want to do as little work as possible in them.

Generally, you only want to use what your template language provides (#each, #if, :else, etc.) and let the other layers do any heavy lifting. In this case, you rely on your templating language to filter your data which is best handled elsewhere. Any complex logic in templates should be considered a "code smell." ;)

One of the most valuable ideas in application design is idea to break your application using the Model/View/Controller (MVC) pattern. (see https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller)

MVC is a way to manage complexity in applications while also embracing the "single responsibility principle" of software design. (see https://en.wikipedia.org/wiki/Single-responsibility_principle)

Aside:
Single Responsibility is the idea that it is always best for your code modules (or other collections) to focus on doing "one thing very well". This is exemplified by the original design of the Unix (Linux) Operating System which included many, many small programs (sed, grep, cat, etc.) that all did only one thing, but did that thing extremely well. The output of one of these programs could then "piped" into the input of others, creating complicated processing in "pipelines." cat logfile | grep -v user | ... which takes the content of a file called "logfile" and supplies it to grep -v user which will only pass those lines that do not contain the pattern "user" to the next program in the line.

Back to MVC:
In MVC, the Model is responsible for managing the application's data, the View for managing presentation, and the Controller for retrieving the data, applying any business rules needed to transform them for presentation, and passing them to a View.

In our application, the *.svelte components are its Views and we use JavaScript classes (like post) to both provide data to them (their Model) and handle any business logic required (their Controller).

So, my inclination would be to drop the /lib/server/post.js in favor of two classes:

• lib/server/announcement.js and
• lib/server/blogpost.js.

So that, in announcement.js, you can provide a listPublishedAnnouncements() method and let graphql do your filtering and sorting for you, as in:

query Post {
    post(
        filter: {
            _and: [{ type: { _eq: "announcement" } }, { status: { _eq: "published" } }]
        }
        sort: "-publish_on"
    ) {
        slug
        title
        body
        publish_on
    }
}

The filter: { _and: [{ type: { _eq: "announcement" } }, { status: { _eq: "published" } }] } clause says: return all records where type is 'announcement' and status is 'published'.

The sort: "-publish_on" clause says: sort the result by the value of the "publish_on" field, descending (indicated by adding a minus at the beginning of the property name.)

The rule-of-thumb when making graphql queries is to only retrieve what you need, when you need it.

So, the listPublishedAnnouncements() method is tailored specifically for the use-case you need, which is retrieving the list of published announcements to be displayed on the "List Announcements" page which only requires the title, slug, publish_on, and body fields.

For other uses, like showing the top five announcements in the landing page block, you might provide a second getPublishedAnnouncementsForBlock() method in which you can limit the number of retrieved records and sort them as you require, as in:

query Post {
    post(
        filter: {
            _and: [{ type: { _eq: "announcement" } }, { status: { _eq: "published" } }]
        }
        sort: "-publish_on"
        limit: 5
    ) {
        slug
        title
        body
        publish_on
    }
}

where the limit: 5 clause says to only return the top five records based on how they are sorted.

Following this approach to provide methods for each specific use-case makes your code directly answerable to a specific requirement.

Also, it should be noted that directus will only return the top 100 records for any query unless you add a limit: -1 clause.

[raisch]

Ahhh, I did not notice that this was in the Announcements block. This review comment is especially pertinent for both /src/lib/server/post.js and /src/routes/announcement/+page.server.js as well.

[raisch]

typo: change posts.slug to post.slug

[raisch]

As I mention above, we do not have a use-case for this. I.e. there is no reason to retrieve all the posts in one go. As I mention elsewhere, this is very wasteful both in time and bandwidth.

[raisch]

As I mention above, Post.getAllPosts() is extremely wasteful in that it retrieves all posts which need to be filtered after retrieval.

[raisch]

Since we have a need for retrieving the content of a specific Post, whether it's an Announcement or a BlogPost, and depending on the specific use-case here, either add getAnnouncementBySlug(slug:string) and getBlogpostBySlug(slug:string) to their appropriate classes, or keep /lib/server/post.js for this specific use-case.

Personally, I think it's cleaner (but a little more typing or copy-paste) to add to the two classes rather than keeping a Post class around for just this purpose. YMMV ("Your mileage may vary" which I use to denote something left up to personal preference.)

[raisch]

Also, should I keep doing pull requests or make a separate branch or something? I've always just committed directly to my own repos so this is new to me

As you can see from my code review, submitting PRs is a great way to learn from other, more experienced, devs. It's completely up to you whether or not you want to keep submitting them, but if you do, I'll keep reviewing them. Good devs know to never take code reviews as criticism, they are not meant personally in any way. Code reviews are recommendations that you can use to become a better coder. In more strict environments than ours (like when working in a dev team in a business), nothing is ever allowed into the production branch ('main') without being reviewed by both peer developers and senior software architects.

As far as branches go, I've found they are most useful when adding a new feature (like carpool) which requires creating/touching/updating a lot of files in one go. They become especially important when adding lots of changes to existing code which have the possibility of "breaking the build." Complex systems become "brittle" (or easily breakable) as time goes on and more hands touch them. This is why having a comprehensive test suite in place to guarantee that new changes don't break stuff is so important.

[raisch]

I'll also add a little here about how to best use git. I've not looked too deeply into your commits, so this is really just general advice, not specific to your work habits.

The rule-of-thumb for git is: Commit early and often.

This means that any change you make to the code base should be committed (with comments) as soon as you make the change, rather than saving them up for, what we call, an "omnibus commit" that makes a lot of changes under a single commit, usually with the comment: "Updated" ;)

So, rather than making a change to '.gitignore', and one to 'package.json', and a third to some js file, and committing them in a single commit, it's a lot better to commit them individually. It doesn't really cost anything to commit them separately and it also makes your intent clearer. So for these three changes, you might commit the first with 'added tmp/', the second with 'refactored run script', and the third with 'removed comments and generally cleaned up'.

[R4z0rBl8D3]

I agree with committing early. It's become a habit of mine to forget to commit when I do something because I realize that I start working on something else and then I have a massive commit by the end of the week. I'll try to commit more.