Feature/meet the host #45
Conversation
| @@ -45,3 +46,4 @@ Currently, two official plugins are available: | |||
| - Michael Kleinschmidt ([@miklesch](https://github.com/miklesch)) | |||
| - Kalina Iwaszko ( [@messkalina] (https://github.com/messkalina)) | |||
| - Bhagya Samarathunga ( [@BhagyaPrasadSamarathunga] (https://github.com/BhagyaPrasadSamarathunga)) | |||
| - Iman Bajalan ([@BajalanIman](https://github.com/BajalanIman)) | |||
There was a problem hiding this comment.
It seems like your feature branch contains changes from the previous ticket. It's not a big deal but it's a good practice to only include changes in your PR which are directly relevant to the ticket. This makes it easier for your teammates to review your code, otherwise they might get confused, or there is just more code that needs to be reviewed.
There was a problem hiding this comment.
When I tried to push my changes, I noticed that the previous conflicts from last night were still unresolved. So, I resolved the conflicts first and then pushed all the changes. I'll definitely keep this in mind for next time.
| </div> | ||
| <hr className="borders" /> | ||
| <div className="reviews-container"> | ||
| <span className="reviews-numbrs">1</span> |
There was a problem hiding this comment.
This seems like a something that could be passed via a prop https://react.dev/learn/passing-props-to-a-component
Even though our goal for now is to show a static view with dummy data, we should still make the component reusable.
There was a problem hiding this comment.
Thank you for the clarification. I assumed that in this step, we are supposed to implement the tickets as simply as possible. Therefore, I used values rather than props. I will store all the data for this component in an object within App.jsx (temporary) and pass it down to the component via props. Is that approach okay?
| </svg> | ||
| </div> | ||
| <div className="raus-superhost-container"> | ||
| <h2 className="raus-info">Raus</h2> |
There was a problem hiding this comment.
It seems to me that "Raus" is a name of a concrete host. We should avoid including such data into things like css class names because eventually this component will be used to display any user or host.
There was a problem hiding this comment.
Thank you so much for this comment. I always have a problem with naming CSS classes :) I will change the names.
| aria-label="check mark" | ||
| className="verifiedicon-svg" | ||
| > | ||
| <title>verified-sheild</title> |
There was a problem hiding this comment.
Probably not all hosts are verified. We could use a prop here too that will tell us if the host is verified and only if the prop is true we display the shield.
There was a problem hiding this comment.
of course. I will do that. Also, as you mentioned in the next comment, this is a screenshot of my work:
|
Great work putting out a pull request so quickly! 🚀 Opening a pull request even before it's fully fine-tuned to every last detail is a good practice because it helps you collect the feedback very early. I often open a PR when I just want to see if the general direction I'm going with is correct. This usually saves me time because I don't have to work on the details if I find out that need to choose a completely different solution. I think the great potential improvement for your PR would be to start using props for the data which will be passed to the component dynamically. You can read more on props in React documentation here https://react.dev/learn/passing-props-to-a-component and if something is not clear you can always ask either by replying here in the PR to the comments or if it's more broad topic we can discuss it in the slack channel. Also, it would be amazing if you could always post a screenshot in the PR when you're working on something visual. |
This is the "Meet your host" component.