Closed andyrew closed 5 years ago
Hi @andyrew, this looks really good! 💯 I wish I had the viewer moved to GitHub sooner.
There is only one issue and that is my fault. I have moved the js libraries into libs
folder but you have kept the new one in the root. I will move them around and send a PR to your fork. Once that is merged we can merge this one in!
Invitation sent! I also just saw that this is your first pull request ever (or that is what GitHub says). Congratulations! 🎉 🎉 🎉 and let me know if you have any questions about merging this PR or any other git / github related confusing terms. 😄
I'm so green, I don't even know what questions to ask. Just please be patient with me if I do something wrong. Thanks, Andy
On Wed, Aug 28, 2019 at 6:47 AM Mostapha Sadeghipour Roudsari < notifications@github.com> wrote:
Invitation sent! I also just saw that this is your first pull request ever (or that is what GitHub says). Congratulations! 🎉 🎉 🎉 and let me know if you have any questions about merging this PR or any other git / github related confusing terms. 😄
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ladybug-tools/bsdf-viewer/pull/4?email_source=notifications&email_token=AATYUYE6KB4SMTPS3UIKZ7LQGZ6XJA5CNFSM4IQ3PZPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5LFVZA#issuecomment-525753060, or mute the thread https://github.com/notifications/unsubscribe-auth/AATYUYBBMH6WXYEX3Y4LHJ3QGZ6XJANCNFSM4IQ3PZPA .
I fixed the issues you pointed out. I reduced some of the font sizes from 16 to 13. I also added toggle buttons for showing and hiding patch numbers to the form. It's commented out at the moment, until I can add the functionality to the viewer. I generally like the look of the viewer without patch numbers, but they're extremely useful sometimes, hence the toggle.
All looks good! My suggestion is to merge this PR in first before adding new changes. Let me know if it will be helpful to do it together for the first time. We can set-up a call and go over the steps. It looks like for this case we can squash and merge all of them as styling with bootstrap - and work on a different PR for show / hide patch numbers.
Hi Mostapha, I've done some simple styling using bootstrap. Have a look and see what you think. Andy
resolves #2