KevinKelly25 / LearnSQL

LearnSQL, a website for learning SQL
1 stars 2 forks source link

Workshop and sandbox user interface for both student and teacher #97

Closed michaeltorres1 closed 5 years ago

michaeltorres1 commented 5 years ago

In this PR I have made the user interface for the workshop and sandbox page for both student and teacher. The point of this PR is just to make a UI for the user to be able to navigate to two text box like terminals with the future purpose being that the user will be able to execute queries in one text box and in the other text box see the result.

After logging in as a student or teacher a workshop navigation tab pops up in the navigation bar. By clicking that tab the user will be directed to the workshop page with a list of databases, in a table, which should be the list of classes they are enrolled in. By clicking on one of the class names the user will be directed to the sandbox. The bottom "terminal" will show the current database they are in and where they will execute queries. The top "terminal" is where the results will be displayed. Please be sure to check that the database name displays correctly for when the user enters the sandbox page.

This PR is just for the UI of the workshop and sandbox page and is not meant to be able to make any connection to execute queries at this point, instead, this feature should be worked on separately.

KevinKelly25 commented 5 years ago

Thank you for the PR @michaeltorres1

I am having an issue where when I click on class as a student nothing happens in the workshop page. It looks like you might have forgot to commit as it looks like there is not a redirect method in html or angular controller.

michaeltorres1 commented 5 years ago

Thank you for spotting this problem @KevinKelly25. Something strange happened when creating the PR. I am not sure how this problem occurred but I pushed what I already had and I checked to see if the file updated. All the files look up to date. Please check to see if this issue still exists.

KevinKelly25 commented 5 years ago

Thank you for fixing that issue, I can now get into the sandbox section. The classes page looks good and is working fine.

I think there could be some improvements: 1) The class name should either be permanently in the input box and not overwritten when inputting query (like psql works) or above the output or title of the page and just say something like enter query where classname currently is. I believe the later might be the better option for now since it is simpler and this UI is only meant to be temporary until we can get a modified version of PSQL to work with our databases or focus on a more complex UI.

2) The output textbox should not allow user to enter text

3) The branch needs to be linted

4) I am not sure there is a reason to create a new sandbox tab and instead just keep workshop highlighted. It is not used anywhere else and seems unneeded.

5) It might be more intuitive for the user if the textbox for entering queries was smaller then the output box. This way it looks like more modern chat apps so users can more easily understand where they need to type queries. Also output will eventually need to be much bigger while input will normally be fairly small but have the option to scroll and make it bigger

michaeltorres1 commented 5 years ago

Thank you @KevinKelly25 for your review. I have made the changes you requested and fixed the issues you found. For the class name in the textarea, unfortunately, I was not able to keep the class name to show consistently when inputting text. In order to show this persistence the textarea has to have a readonly attribute which disallows a user to input text into the textarea. However, I have changed the message to something more intuitive.

In addition to these changes I updated the UI design to contain a run code button under the textarea for future implementation. I also added the learnSQL footer that was missing in the page.

KevinKelly25 commented 5 years ago

Thank you for the additions @michaeltorres1 it looks better now. There is only a few small things I noticed.

cinnaco commented 5 years ago

Thanks @michaeltorres1, the UI seems intuitive and I should be able to interface with it well for this coming milestone. As with every PR, I do my testing using two browsers, Microsoft Edge which I use as my primary and Mozilla Firefox. This ensures that our web application works with EdgeHTML and Blink-based browsers. Since the Blink engine was forked from Apple's WebKit engine, we can assume compatibility with Safari (for now). I do not test with Google Chrome since you guys develop using it. In my search to confirm browser independent code, I ran into an error in the sizing of the textareas on Edge. I found a very simple fix in this case, which is to remove the px unit for the cols attribute for the textareas. These occur specifically on lines 93 and 99 in the sandbox.html file. It should also be noted that the textareas are not resizable in Edge like they are in Blink-based browsers. This does not significantly hamper the functionality and this feature will be added to Edge in a future Windows feature upgrade anyway.

michaeltorres1 commented 5 years ago

Thank you for your reviews. I have fixed the issues that both you guys mentioned. I tested out the UI on different browsers and they work fine now. As per the issues @KevinKelly25 found, I got rid of the getClassInfo function because there was no need for it anymore. Instead I return the classid to be used in sandbox.html for future use to send queries. I also return isTeacher as requested.