Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganise GUI Layout #106

Merged

Conversation

saad-haider-pp
Copy link

Resolves #95

Result bar moved to the bottom. Delivery section fills up remaining space.
image

The padding between the quantity and max quantity has been reduced by half as well.

@saad-haider-pp saad-haider-pp added this to the v1.2b milestone Oct 12, 2020
@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #106 into master will decrease coverage by 1.55%.
The diff coverage is 2.70%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #106      +/-   ##
============================================
- Coverage     67.57%   66.02%   -1.56%     
  Complexity      430      430              
============================================
  Files            77       80       +3     
  Lines          1462     1498      +36     
  Branches        154      158       +4     
============================================
+ Hits            988      989       +1     
- Misses          429      464      +35     
  Partials         45       45              
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/seedu/address/logic/LogicManager.java 71.42% <0.00%> (-3.58%) 3.00 <0.00> (ø)
src/main/java/seedu/address/model/Model.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
...in/java/seedu/address/model/delivery/Delivery.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
src/main/java/seedu/address/ui/DeliveryCard.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../main/java/seedu/address/ui/DeliveryListPanel.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
src/main/java/seedu/address/ui/ItemCard.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/main/java/seedu/address/ui/MainWindow.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...rc/main/java/seedu/address/model/ModelManager.java 92.30% <50.00%> (-1.70%) 23.00 <0.00> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9809b92...77533cf. Read the comment docs.

@@ -12,7 +12,7 @@
<?import javafx.scene.text.Text?>
<?import javafx.scene.text.TextFlow?>

<HBox id="cardPane" fx:id="cardPane" xmlns="http://javafx.com/javafx/11" xmlns:fx="http://javafx.com/fxml/1">
<HBox id="cardPane" fx:id="cardPane" xmlns="http://javafx.com/javafx/11.0.1" xmlns:fx="http://javafx.com/fxml/1">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change to /11.0.1 automated by your intellij system or you changed it manually?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh didn't realise that happened, must have been automatic

<StackPane VBox.vgrow="NEVER" fx:id="" styleClass="pane-with-border"
minHeight="295" prefHeight="295">
<VBox minHeight="-Infinity" prefHeight="440.0">
<StackPane fx:id="deliveryDisplayPlaceholder" minHeight="-Infinity" prefHeight="440.0"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<StackPane fx:id="deliveryDisplayPlaceholder" minHeight="-Infinity" prefHeight="440.0"
<StackPane fx:id="deliveryListPanelPlaceholder" minHeight="-Infinity" prefHeight="440.0"

@@ -50,6 +51,9 @@
@FXML
private StackPane resultDisplayPlaceholder;

@FXML
private StackPane deliveryDisplayPlaceholder;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private StackPane deliveryDisplayPlaceholder;
private StackPane deliveryItemListPlaceholder;

Just to keep it more consistent with itemListPanelPlaceholder, what do you think? Just some minor stuff

@Wincenttjoi
Copy link

Wincenttjoi commented Oct 12, 2020

WIP: as discussed in #95

  • GUI help window toolbar
  • Colouring difference (red vs green)

Please tick once done so that we know when to merge 👍

@saad-haider-pp
Copy link
Author

@Wincenttjoi I've made the changes you requested, plus changed the whole structure of the DeliveryList GUI to match that of the ItemList. (Tonnes of headache cuz debugging GUI is super hard).

I also wrote the basic code in MainWindow.java for initialising the values into the DeliveryList on starting the app, with currently an empty ObservableList being initialised temporarily. The Delivery class I wrote is also currently just a stub with no functionality.

@Wincenttjoi
Copy link

@Wincenttjoi I've made the changes you requested, plus changed the whole structure of the DeliveryList GUI to match that of the ItemList. (Tonnes of headache cuz debugging GUI is super hard).

I also wrote the basic code in MainWindow.java for initialising the values into the DeliveryList on starting the app, with currently an empty ObservableList being initialised temporarily. The Delivery class I wrote is also currently just a stub with no functionality.

Thanks for changing and doing more than required! LGTM

*/
public class DeliveryCard extends UiPart<Region> {

private static final String FXML = "ItemListCard.fxml";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be DeliveryListCard.fxml or sth along that line? Or you plan to merge the different cards into same style hence they both refer to ItemListCard?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching that! yes it should be DeliveryListCard

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have fixed it

@Wincenttjoi Wincenttjoi merged commit 3438e5b into AY2021S1-CS2103T-T12-1:master Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GUI layout to split into 2 sections
3 participants