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

improvement(Depot) #5714

Conversation

lomamech
Copy link
Collaborator

  • Improve display in Allowed Destination Depots

closes #5713

@lomamech
Copy link
Collaborator Author

image

@lomamech lomamech requested review from jniles and mbayopanda May 29, 2021 06:45
Copy link
Collaborator

@mbayopanda mbayopanda left a comment

Choose a reason for hiding this comment

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

Using checkbox and grid for allowed distribution depot is a good idea 👍🏽 it gives to the user much space to interact and it is more readable.

I just made a comment about the use of depotLeft and depotRight.

Is it still necessary to have depotLeft and depotRight while we use just one loop with <div ng-repeat ="r in DepotModalCtrl.depots" class="col-md-6">

</label>
</div>
</div>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If depotLeft and depotRight are just for displaying depots left and right, you could use just one loop such as :

<div ng-repeat ="r in DepotModalCtrl.depots" class="col-md-6">
  <div class="checkbox">
    <label>
      <input type="checkbox" ng-model="r.checked" ng-true-value="1" ng-false-value="0">
      <span translate> {{ r.text }} </span>
    </label>
  </div>
</div>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

I tried with this option, the display stays on one column, my idea on concerning the two-column display was just not to have a very long list

Copy link
Collaborator

Choose a reason for hiding this comment

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

.then(depots => {
vm.depots = depots;

let index = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, you don't need to do this extra index. The second argument of Array.forEach() is an index like you made. So you can do this:

depots.forEach((depot, index) => {

// everything else 

});

@lomamech
Copy link
Collaborator Author

lomamech commented Jun 1, 2021

@mbayopanda can you check again

@lomamech
Copy link
Collaborator Author

lomamech commented Jun 1, 2021

@mbayopanda can you check again

image
@jmcameron I found another way to display on 2 columns
image

@jmcameron
Copy link
Collaborator

@jmcameron I found another way to display on 2 columns

Your approach looks pretty robust and automatic. Nice!

@jmcameron
Copy link
Collaborator

@lomamech How do I get to this display?

@jniles
Copy link
Collaborator

jniles commented Jun 2, 2021

@jmcameron you'll need to enable restricting distribution depots. I think it is in stock settings, but it might be in the depot settings.

Copy link
Collaborator

@jmcameron jmcameron left a comment

Choose a reason for hiding this comment

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

The update looks good and it works fine.

Minor suggestion: since the depots will be listed in two columns, perhaps the create depot modal should be a bit wider?

</div>

<div class="form-group" ng-class="{'has-error' : DepotForm.description.$invalid && DepotForm.$submitted}">
<label class="control-label" translate>FORM.LABELS.DESCRIPTION</label>
<textarea
class="form-control"
name="description"
ng-model="DepotModalCtrl.depot.description"
ng-model=" DepotModalCtrl.depot.description"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like an extra leading space got into this string

lomamech added 3 commits June 3, 2021 09:55
- Improve display in Allowed Destination Depots

closes Third-Culture-Software#5713
- Remove depotLeft and depotRight
- Use index for display depot in two column
@lomamech lomamech force-pushed the improveDiplayAllowedDestinationDepots branch from 2d85831 to a46b9e2 Compare June 9, 2021 13:38
Copy link
Collaborator

@jniles jniles left a comment

Choose a reason for hiding this comment

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

LGTM. bors r+

@jniles
Copy link
Collaborator

jniles commented Jun 9, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 9, 2021

Build succeeded:

@bors bors bot merged commit 211e548 into Third-Culture-Software:master Jun 9, 2021
@lomamech lomamech deleted the improveDiplayAllowedDestinationDepots branch June 28, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improved display of distribution deposits for a deposit
4 participants