Closed Avi-88 closed 1 year ago
Name | Link |
---|---|
Latest commit | 12772be7423401a687d4795196af5a4f772725e3 |
Latest deploy log | https://app.netlify.com/sites/meshery-play/deploys/640dcfef1eef190008022c14 |
Deploy Preview | https://deploy-preview-66--meshery-play.netlify.app/ |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
@Avi-88 the deployment preview failed to build. Would you like to confirm that you're able to build locally? Please double-check on the failure reason.
Hi @leecalcote , there was a unused component which caused the build to fail as netlify treats warnings as errors. The preview is successfully generated now , please let me know if you need any changes, thanks
@roopeshsn will you review and offer feedback?
@roopeshsn will you review and offer feedback?
Sure, I'll do that.
Hey just wanted to add my suggestions here - I don't think a separate mobile menu would be required as we don't have any navigation links we can keep things simple and also with the theme-toggler inside the mobile menu like that doesn't seem to stand out - people won't notice. I was thinking if we can keep it simple and do something like this. it's just a suggestion would really like to know all of your views on this.
Looks good to me. The concern was the code formatting. Also, I am not sure it matters. Other than that all good. Good work @Avi-88!
Sorry for the delayed reply and thankyou for the feedback @roopeshsn , I'll try and fix the formatting ASAP, I'm a little busy atm due to clg work
Hey just wanted to add my suggestions here - I don't think a separate mobile menu would be required as we don't have any navigation links we can keep things simple and also with the theme-toggler inside the mobile menu like that doesn't seem to stand out - people won't notice. I was thinking if we can keep it simple and do something like this. it's just a suggestion would really like to know all of your views on this.
Hi @swagataroy30 , thank you for the suggestion. Although we currently do not have any other navigation within the page, that might not be the case in future. I'm not sure how large the scope of this site will be but it's better to prepare for it. Also about the mode switch being inside menu, it's only made that way because as you said we currently don't have many links and it would look weird to have just 2 buttons in there. As far as it's use, I've seen plenty of similar implementations so i thought it wouldn't be that much of a problem. Still I'm open to hear other people's opinion on this, thanks
If we haven't reviewed this in the Websites meeting, perhaps, a quick review will get this over the hump.
If we haven't reviewed this in the Websites meeting, perhaps, a quick review will get this over the hump.
Sorry about that @leecalcote , this one slipped my mind. There are multiple PR's from my end that have been completed but need a final review before merge , I'll update all of them and bring them up in the next meeting, thanks for the reminder
If we haven't reviewed this in the Websites meeting, perhaps, a quick review will get this over the hump.
Sorry about that @leecalcote , this one slipped my mind. There are multiple PR's from my end that have been completed but need a final review before merge , I'll update all of them and bring them up in the next meeting, thanks for the reminder
Thank you, @Avi-88. I'll see you on tomorrow's meeting.
This PR updates the navbar for smaller screens as proposed in the mentioned issue
This PR fixes #64
Signed commits