Fixes #263 - Added fetching and rendering of data from the backend to the frontend #346

lucacataldo · 2019-11-25T18:27:42Z

Added the fetching & rendering of posts in frontend. This includes fetching post & participant data using jQuery (currently dummy data in src/frontend/dummy_data), and rendering it using a templated

element. I picked some posts from the current planet that included elements I though would bring up some edge cases and adjusted CSS accordingly so that when this gets hooked into the actual routes it'll be a bit less garbled and legible. Also, since pagination.js dialogue.js are no longer relevant or being used, I removed them.

Added the fetching of posts from frontend Made small CSS changes to fix some rendered elements Templated <article> tag for rendering

cindyorangis

So we reviewed your PR in class today and we love what you're doing but agreed that it's way too big for just one PR, this can be broken up into smaller PR's (someone suggested dividing it up into 2 seperate PR's one for fetching and another for rendering). We found a few areas that will be problematic as well

In participants.json, the urls there are not valid, they should be https://google.ca
In posts.json, the guids should not be "null", something like 123 would be better than "null"
These lines

in index.html just disappeared and we don't know where they went or why they were deleted/replaced

cindyorangis · 2019-11-26T18:54:15Z

I also think everything in main.css can be a separate PR by itself as well

miggs125

You have a lot of JS in index.html. This could all be written in a different file and imported in the html file.

src/frontend/dummy_data/participants.json

src/frontend/dummy_data/posts.json

src/frontend/index.html

humphd · 2019-11-27T22:30:09Z

src/frontend/index.html


-        let close = document.querySelector('#mobile-burger-close');
+    <script>
+      $(() => {


JavaScript in .js files please.

Moved to main.js, would appreciate any resources on how to keep code in the front end in modular files as I was struggling with that

src/frontend/style/main.css

lucacataldo · 2019-11-28T04:30:20Z

Thanks for the feedback @cindyledev @miggs125 @humphd its greatly appreciated! Will be putting up a new commit in the coming days addressing the issues brought up.

In regards to having the Javascript in separate files, I had all the code separated into modular files initially (UI tools in one, fetching in another, etc.) but was getting a lot of eslint errors where it was picking up variables as unused between files because they weren't really linked anywhere other than the <script> tags in the HTML. Wasn't able to fix that so would it be alright to have everything in one (separate) .js file for now and maybe have someone file an issue / PR to make things more modular and correctly integrated or adjust eslint settings in the future?

lucacataldo · 2019-11-28T05:47:33Z

So, I've got a commit waiting and ready to go with all the fixes specified including dropping the CSS changes. I previously added these to address UX / UI issues that arose when I added dummy data to better represent typical posts. The following is a screenshot of a mobile view with the proposed changes:

As you can see this will greatly impact the way that posts are rendered.

@humphd @miggs125 @cindyledev
I'll leave it up to you guys as to whether or not to include them or not since I can personally see valid arguments for both including and not including them. 🤷‍♂

I think everything else that was mentioned has been fixed and will put up the PR when I hear back regarding the CSS changes. Thanks again for taking the time to look things over!

humphd · 2019-11-28T14:24:18Z

@lucacataldo the discussions of splitting this up aren't aimed at having you throw changes away; rather, we're trying to improve the reviewability of the work, and isolate changes to their own PRs. Doing the fixes to CSS is important, and we need that. But the question is: do we need it in the same PR as one which adds data fetching?

If it's not possible for you to separate these, then please go ahead and do it all at once. But in future, I want all of us to try and think about how to keep our changes simple, focused on doing only 1 thing, and making it easier for them to get reviewed, tested, and merged.

lucacataldo · 2019-11-28T15:29:13Z

@humphd totally understandable and it was a mistake on my part to go down the rabbit hole so to speak. Will definitely keep this in mind in the future. As for now, I am going to give it a shot to minimize the amount of CSS added in this PR and file a new issue for the other smaller fixes.

Review Changes 1

caught up with upstream

lucacataldo · 2019-11-28T17:56:56Z

@cindyledev @humphd @miggs125 changes have been pushed, have a look and let me know if there's anything that's still outstanding!

cindyorangis

Thanks, I really like that you moved a lot of the scripts in the index.html file to it's own JS file! I'll be keeping an eye on your #366 for the css improvements so lemme know when the PR for that is up :)

humphd

One issue to clear up, the rest looks ready to go.

humphd · 2019-11-28T22:19:57Z

package.json

-    "jest-watch":"cross-env MOCK_REDIS=1 jest --watch",
-    "start": "node src",
+    "jest-watch": "cross-env MOCK_REDIS=1 jest --watch",
+    "start": "node src/backend",


None of these files should be changed in your branch as far as I can tell. We've had people overwrite these already in the past few days, I'm not eager to repeat it. If I'm wrong and you do need these to be updated, let me know why.

humphd · 2019-12-03T19:28:38Z

@lucacataldo see my work in #426, which takes what you have here, and connects it into the new backend we just merged. You could use that to update your code in this PR.

deleted dummy data since we're hooked into the backend now reverted accidental changes to package.json (i think)

humphd · 2019-12-05T17:14:06Z

Closing and continuing in my other PR with new changes added.

Fixes Seneca-CDOT#263

9db5a65

Added the fetching of posts from frontend Made small CSS changes to fix some rendered elements Templated <article> tag for rendering

cindyorangis requested changes Nov 26, 2019

View reviewed changes

miggs125 self-requested a review November 27, 2019 02:05

miggs125 suggested changes Nov 27, 2019

View reviewed changes

humphd requested changes Nov 27, 2019

View reviewed changes

lucacataldo added the area: front-end label Nov 28, 2019

lucacataldo added 3 commits November 28, 2019 10:41

Fixes Seneca-CDOT#263

a2b8253

Review Changes 1

Fixes Seneca-CDOT#263 cleanup

71f9bd2

Fixes Seneca-CDOT#263 - remove temp fix in package.json

d837d72

caught up with upstream

lucacataldo mentioned this pull request Nov 28, 2019

[Preemptive] CSS issues with real-world blog post elements and content #366

Closed

lucacataldo requested review from humphd, cindyorangis and miggs125 November 28, 2019 17:18

Seneca-CDOT#263 - Resolved conflicts for merge

85a65f2

cindyorangis previously approved these changes Nov 28, 2019

View reviewed changes

humphd reviewed Nov 28, 2019

View reviewed changes

lucacataldo dismissed cindyorangis’s stale review via acd9f91 November 29, 2019 00:57

lucacataldo force-pushed the issue-263 branch from acd9f91 to 85a65f2 Compare November 29, 2019 01:09

humphd added a commit to humphd/telescope that referenced this pull request Dec 3, 2019

Taking work by lucacataldo from Seneca-CDOT#346

2e79c5f

humphd mentioned this pull request Dec 3, 2019

Update work for issue 263 to connect to backend #426

Merged

humphd added a commit to humphd/telescope that referenced this pull request Dec 3, 2019

Taking work by lucacataldo from Seneca-CDOT#346

c2ea2fe

lucacataldo added 2 commits December 5, 2019 11:05

Updated using @humphd's integrated code

2631827

added limit to posts being fetched

94c7873

deleted dummy data since we're hooked into the backend now reverted accidental changes to package.json (i think)

humphd added a commit to humphd/telescope that referenced this pull request Dec 5, 2019

Taking work by lucacataldo from Seneca-CDOT#346

e8fc67b

humphd closed this Dec 5, 2019

humphd added a commit that referenced this pull request Dec 5, 2019

Taking work by lucacataldo from #346

9c60192

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #263 - Added fetching and rendering of data from the backend to the frontend #346

Fixes #263 - Added fetching and rendering of data from the backend to the frontend #346

lucacataldo commented Nov 25, 2019

cindyorangis left a comment

cindyorangis commented Nov 26, 2019

miggs125 left a comment

humphd Nov 27, 2019

lucacataldo Nov 28, 2019

lucacataldo commented Nov 28, 2019

lucacataldo commented Nov 28, 2019

humphd commented Nov 28, 2019

lucacataldo commented Nov 28, 2019

lucacataldo commented Nov 28, 2019

cindyorangis left a comment

humphd left a comment

humphd Nov 28, 2019

humphd commented Dec 3, 2019

humphd commented Dec 5, 2019

Fixes #263 - Added fetching and rendering of data from the backend to the frontend #346

Fixes #263 - Added fetching and rendering of data from the backend to the frontend #346

Conversation

lucacataldo commented Nov 25, 2019

cindyorangis left a comment

Choose a reason for hiding this comment

cindyorangis commented Nov 26, 2019

miggs125 left a comment

Choose a reason for hiding this comment

humphd Nov 27, 2019

Choose a reason for hiding this comment

lucacataldo Nov 28, 2019

Choose a reason for hiding this comment

lucacataldo commented Nov 28, 2019

lucacataldo commented Nov 28, 2019

humphd commented Nov 28, 2019

lucacataldo commented Nov 28, 2019

lucacataldo commented Nov 28, 2019

cindyorangis left a comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

humphd Nov 28, 2019

Choose a reason for hiding this comment

humphd commented Dec 3, 2019

humphd commented Dec 5, 2019