Skip to content
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

624 recent updates feed on main page #626

Merged
merged 23 commits into from
Aug 9, 2024

Conversation

chance-stribling
Copy link
Contributor

Updating web to include the new ash survey fields

@chance-stribling chance-stribling linked an issue Apr 29, 2024 that may be closed by this pull request
Copy link
Contributor

@florence-77 florence-77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting some backend changes, I'll test the frontend later. Great progress.

app/Http/Controllers/TreetController.php Outdated Show resolved Hide resolved
/**
* Store a newly created resource in storage.
*/
public function store(Request $request)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you already have a create method you don't need store, this method can be deleted.

/**
* Display the specified resource.
*/
public function show(Treet $treet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show is a method that fetches one instance of the model. For example, clicking on a blog post title to view the full post. Since we don't need such a method in this context, this can be deleted.

/**
* Show the form for editing the specified resource.
*/
public function edit(Treet $treet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to be able to edit, but it's not too important at the moment, since we have database access if we really need to edit the text. This method can be deleted.

/**
* Update the specified resource in storage.
*/
public function update(Request $request, Treet $treet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a method for update and edit, I would expect those to both perform the same function. You really only need one, if any. Since we don't need this you can just delete this method.

{
//
}
public function getTreetFeed(Request $request)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code belongs in index(). On that note, take some time to learn about CRUD, a general design model. At a very basic level, the idea is that there are four basic operations we perform on data - Create, Read, Update, Delete. This means one method for each of those operations. In our apps we add one more, a method specifically for fetching multiple, and we call that index(). So, almost all of our controllers contain index(), show(), create(), update(), and delete(). Some purists even argue that if you need more methods than that per controller, you need to refactor! I think that's very extreme. But following the model, most of the time, is helpful I think.

app/Http/Controllers/TreetController.php Show resolved Hide resolved
@florence-77
Copy link
Contributor

Looking very good! Just a few notes:

  • I'm seeing a console error: Warning: Each child in a list should have a unique "key" prop. Check the render method of 'TwitterFeed'. This is resolved easily by adding a unique key attribute to whatever you're iterating over and creating multiple copies of, in this case <Treet>.
  • I think instead of listing time since creation, we should print the date of the Treet, e.g. instead of "3 weeks ago", instead write "Jul 14th. 2024" (you can abbreviate the month). That seems more appropriate in this context.
  • Additionally, to save space and make each Treet more compact, let's put the date in the top right, opposite the app name.
  • Let's make clicking on the icons/site names (HealthyWoods, FlorestaDB, etc.) open that site in a new tab.

@florence-77 florence-77 merged commit ec22cbe into master Aug 9, 2024
@florence-77 florence-77 deleted the 624-recent-updates-feed-on-main-page branch August 9, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Recent Updates" feed on main page
2 participants