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

Set up Issue endpoint and page #75

Closed
4 tasks done
Cleop opened this issue Mar 9, 2018 · 6 comments
Closed
4 tasks done

Set up Issue endpoint and page #75

Cleop opened this issue Mar 9, 2018 · 6 comments

Comments

@Cleop
Copy link
Member

Cleop commented Mar 9, 2018

Relates: #14

As a user who has clicked on the meta table on github
I want to be able to see an overview of the history of my issue on the backup site
So that I can dig deeper into the comments/histories of interest to me

  • Create issues endpoint/page
  • Get the comments text from s3
  • Get the comment details from the DB
  • Display all of the comments of a given issue id on that page

@nelsonic - should we show every version of every comment or just the latest version of each comment?

@Cleop Cleop self-assigned this Mar 9, 2018
Cleop added a commit that referenced this issue Mar 9, 2018
@Cleop Cleop removed the in-progress label Mar 9, 2018
Cleop added a commit that referenced this issue Mar 9, 2018
@nelsonic
Copy link
Member

nelsonic commented Mar 9, 2018

@Cleop most comments will not be edited and the ones that are will only have a few edits.
For now let's just display everything on a single page to reduce the amount of "functionality" we need to have i.e. a single page for everything for that issue.

@nelsonic nelsonic added this to the MVP milestone Mar 12, 2018
Cleop added a commit that referenced this issue Mar 12, 2018
@Cleop
Copy link
Member Author

Cleop commented Mar 12, 2018

This is now working however, credo is complaining about the duplicate functions that exist in the issue and comment views. So now I'm going to read up on how to get rid of this error by sharing the functions.

This article talks about sharing functions between controllers but not specifically between views, I don't quite think it's what we're after here:
https://medium.com/@cjbell_/sharing-methods-between-phoenix-controllers-358ab6d36e22
There's nothing explicit in the docs:
https://hexdocs.pm/phoenix/views.html

This is a closer match:

'How to create a global view'

https://stackoverflow.com/questions/32755415/how-to-create-a-global-view-in-phoenix-framework
and this:

https://elixirforum.com/t/importing-a-function-from-sharedview/7241/4

'Importing a function from SharedView'

What is SharedView? - see https://hexdocs.pm/phoenix/templates.html

SharedView is for shared templates. It’s not ideal to share functions in it with other views. To share function just copy what phoenix does with ErrorHelpers. There’s a ErrorHelpers module in the views folder and that module is imported in the web.ex file for views. ErrorHelpers is a plain elixir module, so you don’t have that circular import like with SharedView.

Another option is to go ahead and use SharedView but don’t insist on importing the function and instead alias SharedView. It means all the callsites will be SharedView.my_func() but it’s more explicit and clearer to people new to the codebase where that function is defined.

I'm not sure if SharedView is for functions or meant only for templates though...

'It’s a good practice to move these templates into their own shared directory' from the hexdocs.

I don't think SharedView is right because it's meant to be in your templates folders: lib/hello_web/templates/shared and I'm talking about sharing functions not templates.

I think I'm going to try the global view concept.

@nelsonic @RobStallion @SimonLab any thoughts?

@Cleop Cleop added T1d and removed T2h labels Mar 12, 2018
@Cleop
Copy link
Member Author

Cleop commented Mar 12, 2018

I tried to implement the stackoverflow global view concept by adding:

 # Import custom helpers
 import AppWeb.IssueView

(I will rename it later)

but am getting this error, the same error that the elixir forum question was talking about:

image

This was the given reason for this error on the elixir forum question:

Since your SharedView module also has this quote block injected, it’s trying to import itself, which is causing the error. The easiest option is to my those function, i.e. my_func/1 to another module that you import in the view block instead of defining it on SharedView

However, I don't fully understand the solution.

Also, I don't think I've done what was at the bottom of the stackoverflow implementation here as I didn't really understand what they were saying:
image

@Cleop
Copy link
Member Author

Cleop commented Mar 13, 2018

Managed to fix this one with @RobStallion.

We've created a helpers folder in app_web which contains the functions:
image

Then each view is requiring this file in:
image

We also tested this by putting it in web.ex which worked, so if we want them globally we can do this in the future.

@RobStallion
Copy link
Member

@Cleop Sorry for the delay getting back to you.

nelsonic added a commit that referenced this issue Mar 13, 2018
@nelsonic
Copy link
Member

@RobStallion thanks for helping! 🎉
@Cleop looks good! 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants