search

vincentahn / clamor

1 stars 0 forks source link

Front-end Routes suggestions #4

Open brtrick opened 3 years ago

brtrick commented 3 years ago

Good job with the front-end routes. A few comments/suggestions:

  1. What discord page do you see servers/:serverId corresponding to?
  2. Do you plan to have an "explore servers" page? (would be an index of servers)
  3. Will your channel ids be unique across the app? If so, then no need to nest the channel id under the server id (/channels/:serverId/:channelId).
  4. Will you have a page section showing the server members (or is that what you mean by UserSettings?)?
  5. Just be aware that the ServerForm (and ChannelForm? I've never created a discord channel so I'm not sure what that looks like...) should be a modal.
vincentahn commented 3 years ago
  1. I haven't finished rewriting my routes but I plan to only have /, /login, /signup, /channel/@me, /channels/@me/:channel_id, and /channels/:server_id/:channel_id.
  2. I will probably need to add a ServersIndex at least in the interim and include a Servers option in ChannelIndex in '/channels/@me'.
  3. Yes and there is likely no need to nest the channel id under the server id. I do wonder if the discord channel ids are unique. In my case I'm planning on doing so for simplicity with routing.
  4. I wasn't but I really should add a MemberIndex to show that. Especially because this will be useful or needed when/if I implement a feature indicating which users are online.
  5. I don't exactly know what you mean by modal but if you're saying it should be one component that changes depending on the props it receives (i.e. formType) then I'll try to factor that into consideration.
vincentahn commented 3 years ago

Sorry for the delay! I believe I've finished editing my Front-end Routes.

brtrick commented 3 years ago

Looks good. A couple comments:

  1. :channel_id is still nested under :server_id. Is that necessary/useful?
  2. a modal is a pop up that covers part of the page. So, when you click to create a new server, discord does not take you to a new page, but a little box pops up with a form that begins the server creation process.
vincentahn commented 3 years ago

:channel_id is still nested under :server_id. Is that necessary/useful?

a modal is a pop up that covers part of the page

vincentahn commented 3 years ago

Also, I don't know if this is relevant but I've called CurrentUserForm an overlay (covers entire page) but don't currently plan on having it accessed by route to a different frontend url.

brtrick commented 3 years ago

"ChannelIndex will properly show the correct messages according to server"

Unless channels have messages on different servers (which I believe would require more internal tracking than is currently in the design), then knowing the channel_id should be enough. All the messages on that channel will be for the server on which it appears. In other words, while a give channel will be associated with a specific server, once you have the channel_id, that channel_id is all you need to grab everything else associated with the channel.

vincentahn commented 3 years ago

"ChannelIndex will properly show the correct messages according to server"

"ChannelIndex will properly show the correct [channels] according to server".

This is why I said: "Otherwise I could add server_id as an additional field within textChannels in store state". I assume I would need server_id such that ServerIndex will know what text channels (and eventually voice channels) to display.

brtrick commented 3 years ago

I assume I would need server_id such that ServerIndex will know what text channels (and eventually voice channels) to display.

Your state already has that info stored under the servers slice in the channel_ids field. You don't want to store info in more than one place (because then it could get out of sync).

vincentahn commented 3 years ago

So then does that mean that I should have the server_id accessible through the frontend url? (/channels/:serverId/:channelId) Because otherwise to get the serverId I would need to potentially iterate through all servers until I find the correct servers. Is that correct?

brtrick commented 3 years ago

Ok, I think I understand where you are coming from. You're including the server_id in the route, not because you need it for the channel_id, but because you need a way to keep track of which server is active. That could work. Another possible solution to consider: storing the active server_id in session.

vincentahn commented 3 years ago

Thank you for the response! I think I'll stick with keeping the server_id in the route for now but if I run into any issues I'll keep in mind that I could store the active server_id in session.

vincentahn commented 3 years ago

I also wanted to ask about the encrypted credentials.yml file. I'm trying to include pictures with my users now but I wanted to ask if there is any concern with adding the credentials.ymc.enc file to my remote repository. I'm not certain of how the encryption works so I don't know if decrypting my keys is easy for malicious viewers. Should this be included in my .gitignore?