@remyboire
Posted
Hi @Aadv1k, as you request it, here is my feedback.
Your integration looks nice and everything is working properly, great job!
However, as an old art director, I would be worried by some little differences between the design and your integration: colors not always match (the current day bar is more of a blue and the section background is a cream), border radius are 20px on desktop and 10px on mobile (and for the bars, its 5px / 3px). There are also some breakpoints on padding, bar spacing, logo width…
I also noticed that you used width: 14% on bars, which in my opinion not the best way because it assumes that we have 7 bars. This can become a problem in the future if you want to display more or less bars! Personally, I used width:100%, that way you can add or remove bars without breaking the layout.
Regarding the tooltips, I think using :before and :hover would have been an easier way to handle it than with event listeners. You can put a data-attribute on the bar and access it from a pseudo-element like :before { content: attr(data-attribute) } and get rid of the span.
Those are some minor details, but important in my opinion. :)
Marked as helpful
@Aadv1k
Posted
@remyboire Thanks for your comprehensive review! So the colors, I just eyeballed some of the text colors using tailwindcss' builtin color pallete (slate and stone) so there is that, and as for the tool tip THANK YOU, I was wondering if there was a way to use pseudo classes here, I wasn't aware of the content: attr(). Although, I feel like using spans you have a bit more control (although I didn't implement it) for example making it so when you clikck, the span stays there and when you click it is removed (along side hover) but hey, I appreciate your input! Actually Let's connect on github, I'll drop you a follow ~ Aadvik
@remyboire
Posted
@Aadv1k You still can toggle a class in JS for the pseudo-element to stay, I don't think it would pose a problem! Sure let's connect on Github :)
