Issue #48

[negar-75]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
before-i-die-achievements	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 2, 2023 10:54am

[XanderRubio]

Agreed, it makes sense not to have all the emojis displayed, as this is distracting when reading the contributor's message. Great job adding the useState hook for randomColor; it works great when testing, and I am impressed! I see the useEffect hook generates a random color and sets it as the randomColor state whenever the isEnlarged state changes. I like how you have set up your setRandomColor function; from my calculation, there are over 16 million different random color combinations. Translating to lots of variety when clicking👏🏻

[XanderRubio]

@negar-75 Great work, and I will be continually reviewing today and adding empowering feedback 😎 I am impressed with your quick execution on bringing the chalkboard theme to life and the randomization of the chalk colors when viewing contributor's text. You have added much value to the presentation of this project for it to continue to evolve. I'm looking forward to continually directing your code changes. Have a great day, and thank you for your patience in my review and merging.

[XanderRubio]

That makes sense, and thank you for refactoring the MasonryLayout,jsx.

[XanderRubio]

@negar-75 When viewing the added code in this MasonryBox.module.css it makes sense why it is constructed how you have it. I would like to get your thoughts on the layout of the Before I Die..... text and the contributor's text in the test box. In terms of visual appearance, do you believe it looks better centered? Please refer to the photos below⬇️
(Currently)

With Center:

[XanderRubio]

Setting the:
overflow-y: auto;

Suggested change

	overflow-y: scroll;
	overflow-y: auto;

This will eliminate the side scroll bar that is currently popping up (see picture below and this sidebar currently, when a contributor text is short, will be displayed weirdly. I believe it is best to set it to auto for simplicity. Please let me know what you think about if you have another solution to prevent the scroll bar from moving inwards on the text box when a contributor's text is short.

[XanderRubio]

@negar-75 Great work, and thank you for your time making the UI contributions! Additionally thank you for your organized commit structure. Please let me know your thoughts on the comments I have left, and I look forward to your follow-up and merging and closing this issue! Have a great day!

[negar-75]

Hi @XanderRubio
thanks for your great comments about my works, it makes me confident.
in terms of scrollbar view and the picture you sent, in my browser everything is diff


erent,
this is what I see.
but for solving this issue and make it more sensible , we can add a condition for it ,
like if we have more than specific number or letter add scrollbar

[negar-75]

@XanderRubio and also we have another problem that whenever we refresh our page we get the message to download heart-beat.wav

[XanderRubio]

Hi @XanderRubio thanks for your great comments about my works, it makes me confident. in terms of scrollbar view and the picture you sent, in my browser everything is diff erent, this is what I see. but for solving this issue and make it more sensible, we can add a condition for it , like if we have more than specific number or letter add scrollbar

Yes, you are correct @negar-75. I'm not sure why, but when I was reviewing earlier and viewing the deployment preview, I saw the scroll showing up before, and it was affecting the contributors with short text and shows up weirdly now when I view it shows up just as you share your screenshots. My apologies about that. Is your version showing a different formatting for how the images show? The following videos are the current version, followed by the draft version for this issue that is viewed on the preview link for this pull request #52. The draft version preview link has the images showing farther to the left and not completely centered. I want to check if you see the same on your local development server. Thanks 😎

Current Version of Before I Die Code:
https://github.com/BeforeIDieCode/BeforeIDieAchievements/assets/120526253/f21ab0f5-ab81-45c1-bd53-468fc2aabf66

Draft version:
https://github.com/BeforeIDieCode/BeforeIDieAchievements/assets/120526253/a3cbfce7-6e34-47af-892d-47ff54b7593c

[XanderRubio]

@XanderRubio and also we have another problem that whenever we refresh our page we get the message to download heart-beat.wav

I see this message in the console. Is this what you are referring too?

[negar-75]

@XanderRubio
I did not do any changes to view of grid in main page,
just I did work on before I die textbox and for me the main page is same as your current page video


this may be related to Masonry layout?

[negar-75]

and about translation depend on IP,
we can open another issue on that and let me work on this.

[negar-75]

thanks for precise review and detailed comments,
let me know about everything what you think it should be

[negar-75]

@XanderRubio

I am working on auto translation of "Before I die" heading based on IP
but for the "Before I die" contributor's text I have no idea that how can we approach it.

[XanderRubio]

@XanderRubio I did not do any changes to view of grid in main page, just I did work on before I die textbox and for me the main page is same as your current page video this may be related to Masonry layout?

Thank you for clarifying @negar-75 . It seems that the issue may be related to how the project is currently being deployed on Vercel. However, I would prefer to deploy it using GitHub. It's possible that the issue I was seeing in my browser was due to this deployment method. If everything appears normal in your local development environment, that's great. Let's proceed with merging to the main branch and see how it looks. We can always fix any issues or revert the commit if necessary.

[XanderRubio]

and about translation depend on IP, we can open another issue on that and let me work on this.

Makes sense. Can you go ahead and create an issue please for this 😎?

[negar-75]

@XanderRubio I did not do any changes to view of grid in main page, just I did work on before I die textbox and for me the main page is same as your current page video this may be related to Masonry layout?

Thank you for clarifying @negar-75 . It seems that the issue may be related to how the project is currently being deployed on Vercel. However, I would prefer to deploy it using GitHub. It's possible that the issue I was seeing in my browser was due to this deployment method. If everything appears normal in your local development environment, that's great. Let's proceed with merging to the main branch and see how it looks. We can always fix any issues or revert the commit if necessary.

however I checked the position of grid and saw there is padding in the left side ,
maybe by changing padding or margin or by making parent element to flex display make it always center in all browsers,
but I am not sure
what do you think?

[XanderRubio]

@XanderRubio

I am working on auto translation of "Before I die" heading based on IP but for the "Before I die" contributor's text I have no idea that how can we approach it.

This makes sense and great idea to implement. Once we get this merged, we will review and create an issue specifically for the contributor text to translate based on IP and I would need to do research, or another contributor with experience will be able to assist. This is great, though as I didn't consider the translating based on IP, and appreciate you thinking ahead @negar-75!

[XanderRubio]

@XanderRubio I did not do any changes to view of grid in main page, just I did work on before I die textbox and for me the main page is same as your current page video this may be related to Masonry layout?

Thank you for clarifying @negar-75 . It seems that the issue may be related to how the project is currently being deployed on Vercel. However, I would prefer to deploy it using GitHub. It's possible that the issue I was seeing in my browser was due to this deployment method. If everything appears normal in your local development environment, that's great. Let's proceed with merging to the main branch and see how it looks. We can always fix any issues or revert the commit if necessary.

however I checked the position of grid and saw there is padding in the left side , maybe by changing padding or margin or by making parent element to flex display make it always center in all browsers, but I am not sure what do you think?

Yes, setting the parent element's display property to 'flex', and then justifyContent and alignItems properties to 'center', should display the div of the images center on the page. Can you recommit one more time with what you currently have so we can preview the deployment link again? Thank you for your time in the manner and I want to see as I review the MasonryBox.jsx file again and noticed that there aren't any changes that, from my perspective, would cause a shift in the image alignment from your addition of code. It is one of those weird things and grateful as it always provides a learning and deeper understanding of development.

[negar-75]

So what should I do now in branch issue#48?

[XanderRubio]

So what should I do now in branch issue#48?

@negar-75 I went ahead and merged and noticed the formatting was still off when viewing on the live link.

I then went ahead and reverted the pull request to get the main branch back to how it was before.

When viewing on your local set-up, please have a look at the current preview link here and see how the formatting of the images looks for you. Many open source projects will have a deployed version of a preview of what is being merged into the main branch, and this is good to always check before as it can be different sometimes from a developer's local dev server.

[XanderRubio]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
before-i-die-achievements ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 2, 2023 10:54am

Go ahead and click on Visit Preview to see the currently deployed version from your pull request. I'm grateful for your time in this and also for creating the ability for us to learn during this process. This is what I would recommend you do:

1. Get the images displayed centered on the preview link as they did on the current live link on the main branch
2. Create another pull request with this formatting showing, and it will additionally have all of the changes you have done this far for this pull request of the text, chalk, formatting, random color generate function, etc.
3. Then we will be able to merge and get it integrated into the main branch 😎

Please let me know if you have any questions or feedback, Negar.

[negar-75]

So what should I do now in branch issue#48?

@negar-75 I went ahead and merged and noticed the formatting was still off when viewing on the live link.

I then went ahead and reverted the pull request to get the main branch back to how it was before.

When viewing on your local set-up, please have a look at the current preview link here and see how the formatting of the images looks for you. Many open source projects will have a deployed version of a preview of what is being merged into the main branch, and this is good to always check before as it can be different sometimes from a developer's local dev server.

this is what I see in my local system of vercel link

I am using chrome and I checked in firefox and grid is in the middle as well

[XanderRubio]

So what should I do now in branch issue#48?

@negar-75 I went ahead and merged and noticed the formatting was still off when viewing on the live link.
I then went ahead and reverted the pull request to get the main branch back to how it was before.
When viewing on your local set-up, please have a look at the current preview link here and see how the formatting of the images looks for you. Many open source projects will have a deployed version of a preview of what is being merged into the main branch, and this is good to always check before as it can be different sometimes from a developer's local dev server.

this is what I see in my local system of vercel link I am using chrome and I checked in firefox and grid is in the middle as well

That is strange🤔 Because I know we didn't make any changes that would shift the div that holds contributor's images and send them off to the left. Very odd, and thank you for confirming from your perspective and viewing in FireFox also. I did notice after cloning your forked code and then viewing between the main and your issue-#48 branch that the div is flex see image below.

Additionally, here is my perspective when running your code on my local React server, and the first time you see the the view of the Before I Die page is on the main branch of your forked repo and then when I switch to the branch issue-#48 the div element for the images doesn't show flex anymore which is strange since the .flex was never changed. Very odd.

Untitled.1.mov

[negar-75]

about the scrollbar view when I am using firefox I do not see scrollbar

but when I switch to chrome the scrollbar has been shown,

so this is because of browsers

[XanderRubio]

Great insight and this is good to know.

about the scrollbar view when I am using firefox I do not see scrollbar

but when I switch to chrome the scrollbar has been shown,

so this is because of browsers

This is a valuable insight that presents opportunities for future work. One possibility is to use a library like Normalize.css to standardize our CSS. We should also review our CSS styles with regards to scrolling and apply browser-specific styles or scripts as needed in the future. To simplify matters, let's remove the scroll for now to ensure that your contribution can be merged. Additionally, we can specify a maximum character count in the contribution guidelines at this time, so that a scroll isn't necessary for long text inputs.

[negar-75]

hey @XanderRubio
I think I solved the scrollbar issue, you can check it ,
as far as I understand , non-webkit-browsers like firefox ,puts scrollbar whenever the text height exceeds from the div height ,
but for webkit browsers maybe it is not like this,
also I did some changes related to this issue,you can check it out

[negar-75]

so, because I am new with git stuff, what is the next step to merging pull request issue-#48?

[XanderRubio]

hey @XanderRubio I think I solved the scrollbar issue, you can check it , as far as I understand , non-webkit-browsers like firefox ,puts scrollbar whenever the text height exceeds from the div height , but for webkit browsers maybe it is not like this, also I did some changes related to this issue,you can check it out

Great this makes sense on the web scroll from viewing your pull request #59!

[XanderRubio]

so, because I am new with git stuff, what is the next step to merging pull request issue-#48?

All good! Git can be confusing, and there are many times I still need to take a moment to wrap my head around what is the current Git situation. In this case, I would recommend going to your fork repo and opening up a new pull request and you should have no issue opening as the main branch of the Before I Die code will be different than the version you will be adding. Once you do this, check again for me on the deployment link preview as I'm still seeing the images off to the left. It seems you already did this with the recent #59 pull request.

[negar-75]

should I create new pull request here for issue #48 ?
so what happened to changes that I applied to textbox and color randomization before?

[XanderRubio]

should I create new pull request here for issue #48 ? so what happened to changes that I applied to textbox and color randomization before?

@negar No need to resubmit your changes after I reverted the merge into main. Your pull request #59 already includes the textbox and color randomization changes from before. Pull request #59 is connected to issue #48, so once we merge your branch into main it will automatically close that issue.

Reverting the merge in #52 was just to undo the image alignment changes, which weren't quite centered. When I reverted, it went back in time to the previous version of main before the merge. This doesn't affect your code in the issue#48 branch or pull request #59 at all. Your pull request still contains the same changes as before.

Overall, you followed the correct Git flow. I know it can be confusing when merges are reverted, but just know your changes are still safe in your pull request. Let me know if you have any other questions!