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

created sculpture-feed action bar, comment drop down button, comment … #111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mollybeach
Copy link

…submit button functioning, comments only load on refresh rn, comment likes button functioning, sculpture action link button functioning, comment-post delete button almost functioning, click on drop down for comment drops all of the sculptures drop down buttons need fix, created temp db

sp-testing-action-bar-main.mov

…submit button functioning, comments only load on refresh rn, comment likes button functioning, sculpture action link button functioning, comment-post delete button almost functioning, click on drop down for comment drops all of the sculptures drop down buttons need fix, created temp db
…nger opens all of the element's feeds, temporary comment post without refresh
@PWhiddy
Copy link
Member

PWhiddy commented Aug 17, 2021

This is cool! Looking good overall.
Couple high-level thoughts:
All together this is quite a few changes, I think it will be easier for now to merge just the like button and maybe share, and am going to recommend opening the commenting feature #29 in a separate PR.
We should discuss how comment layouts should work. Whether they should be visible from the grid view, and if they will need more space on the page to scale to more than a few comments. I also think we're probably trying to keep the coloring of the website as neutral as possible to minimize interference with the sculptures.
Left a first pass of quick comments below mostly just pointing out unnecessary files and asking some questions.

@@ -0,0 +1,49 @@
_redirects,1629170804545,f7d133168911c500fa3856b83dc84801ba9af7471e4a2b33eef993a066a18632
Copy link
Member

Choose a reason for hiding this comment

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

the files in .firebase don't need to be added to git

@@ -0,0 +1,16 @@
# This file was auto-generated by the Firebase CLI
Copy link
Member

Choose a reason for hiding this comment

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

This deploys a test db when a pr is opened? Do we need this? If so what does the testing workflow look like

@@ -0,0 +1,7 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Is this just for testing? Not sure how this would effect the production deployment

match /databases/{database}/documents {
match /{document=**} {
allow read, write: if
request.time < timestamp.date(2021, 9, 15);
Copy link
Member

Choose a reason for hiding this comment

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

Allow reads and writes if the date is before sept 15?

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused here since we're using firebase real-time and not firestore.

@@ -0,0 +1,14 @@
module.exports = {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you're planning on using firebase functions but no need to commit any of these files in functions in the PR branch if you're not using them yet.

@@ -1,5 +1,5 @@
{
"name": "Shader-Park-Website",
"name": "shader-park-website",
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -18773,7 +18773,7 @@ function getDevicePixelRatio(ctx) {
Object.defineProperty(exports, '__esModule', {
value: true
});
exports.isCommented = isCommented;
exports.iscomment = iscomment;
Copy link
Member

Choose a reason for hiding this comment

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

any reason for changing this?

<button @click="toggleItem(index)" class=" editor-button sculpture-comment-count fade-opacity " style="margin-left: none" ></button>
<span >{{commentCount}} </span>
</div>
<!----------LIKE BUTTON----------->
Copy link
Member

Choose a reason for hiding this comment

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

I like the comments although although my personal style preference would not be all caps and fewer dashes

@@ -0,0 +1,4 @@
Debugger listening on ws://127.0.0.1:59691/bf83f84f-e5d6-4f33-976c-7345f1ad9045
Copy link
Member

@PWhiddy PWhiddy Aug 17, 2021

Choose a reason for hiding this comment

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

remove this file

Copy link
Member

@torinmb torinmb left a comment

Choose a reason for hiding this comment

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

This is so cool! This is something we've been looking to add for a very long time.

I have similar thoughts to Pete. I'd love to talk through some of the UX of the comments, and what happens when there are many comments. Also, how should we show comments when you're viewing the interactive sculpture?

I also agree it'll be a lot easier to start incorporating these changes by splitting these into separate PRs.

@@ -11,13 +11,16 @@
<div ref="threeCanvas" class="canvas-container" :class="{dragging: dragingMouse}"></div>
<div class="actions-bar"></div>
<actionBar :cachedWidth="actionsBarWidth" :dragging="dragingMouse" :class="{dragging: dragingMouse}"></actionBar>
<sculptureFeed :cachedWidth="actionsBarWidth" :dragging="dragingMouse" :class="{dragging: dragingMouse}"></sculptureFeed>
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be loading in the whole sculpture feed here. This component is intended to view and edit 1 sculpture.

});
},

favorite(sculpture) {
Copy link
Member

Choose a reason for hiding this comment

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

We should be turning the action bar buttons into their own .vue components so that they can have encapsulated code. Let's start by making a "FavoriteButton.vue" that has a favorite function we can call. We can easily re-use the button on this page and also in the action-bar on the MainContainer.vue

Copy link
Member

Choose a reason for hiding this comment

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

Right now we end up with a lot of duplicated code between the actions bar that is getting brought into the SculptureFeed

@@ -76,6 +76,7 @@ export default {
setUser: function() {
this.$store.dispatch('setUser');
this.$store.dispatch('fetchUserFavorites');
this.$store.dispatch('fetchUserComments');
Copy link
Member

Choose a reason for hiding this comment

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

^we fetch the current user favorites to know which sculptures to fill in the 'heart' for when you're viewing sculps. I still need to figure out a more efficient way of doing this.

The only reason I'm imagining you'd need to do this with comments is to figure out which comments you need to display an 'X' next to in order to delete. Since you include the UID of the current user in the comment you shouldn't need to load every comment a user makes.

@@ -105,7 +107,7 @@ export default {
</script>


<style lang="less" scoped>
Copy link
Member

Choose a reason for hiding this comment

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

I worry about removing the scoped styling because all of those css styles are in the global namespace now.

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.

3 participants