feat: allow to add port mappings when image has no exported port (#1046)

[lstocchi]

What does this PR do?

Allow to add port mappings when starting an image that has no exported ports.

Screenshot/screencast of this PR

If there is no exported port, the user is free to add multiple local:remote port mappings

What issues does this PR fix or reference?

fixes #1046

How to test this PR?

1. Pull rancher/hello-world
2. Start it

[slemeur]

Nice job @lstocchi !

If we want to be consistent in term of UI/UX with the other options, shouldn't we have the same type of layout with two columns and the '+' button?
Left column would be host port and right column container port? WDYT

[lstocchi]

If we want to be consistent in term of UI/UX with the other options, shouldn't we have the same type of layout with two columns and the '+' button? Left column would be host port and right column container port? WDYT

Sure, now that you make me think about it, yes 😓 . I didn't notice it's the same exact case of adding new envs, for example.

[slemeur]

Also, if in the container definition there are already exposed port, we are showing it like this:

Maybe, we should make that consistent - and allow the user to add additional port mapping even if the image has some pre-defined exposed port?

[lstocchi]

Maybe, we should make that consistent - and allow the user to add additional port mapping even if the image has some pre-defined exposed port?

@slemeur It's certainly a use-case that could happen, do we want to always show those fields or I add a button that creates new fields when requested?

This is static, always visible. Maybe we should add a sentence to explain it

By adding new fields dynamically

WDYT? Do you have other ideas?

[slemeur]

I really like the dynamic approach you choose. Works well IMO.
Let's maybe clarify the tooltip on the "+" action?

[benoitf]

do we need to add a small text/subsection before the ports exposed from the EXPOSE instructions in the Dockerfile
And then add 'additional exposure/ports' where we can add new ports ?

[deboer-tim]

To me it isn't obvious looking at the UI that clicking on the [+] would create another port vs doing something on the existing port 80/9000, so I wouldn't have even looked at the hover help. If there are two existing ports it would probably be more confusing, so I would +1 Florent's comments to separating it and making it more obvious in the UI (but keeping it simple/compact is good).

I'm the kind of person that notices the volume, port, and environment variables all have two text fields and the middles are at three different y positions. :) The local 80/9000 port essentially creates a fourth. Would appreciate it if you could fix at least some of this, and line up the new port columns with something existing.

[lstocchi]

New day, new proposals

Based on your feedbacks I see this in two ways.

The first is just a static section which is always visible. Maybe we can indent the title and inputs a bit to the right to show they are children of the parent port mapping section.

the second is an evolution of the dynamic button.
Added a text so everyone notices it. When clicked extra inputs are shown. If clicked again all inputs and their value are removed. It's like a reset.

WDYT? @slemeur @benoitf @deboer-tim

[benoitf]

I like option 2 but maybe the add remove custom ports button should be on the left ( Or maybe it's a bad idea as well)

[lstocchi]

I like option 2 but maybe the add remove custom ports button should be on the left ( Or maybe it's a bad idea as well)

This is when displayed on the left

[benoitf]

yes it may be better like that especially when we don't have any EXPOSE instruction

[benoitf]

It looks like the codebase is still for the yesterday behaviour

[lstocchi]

It looks like the codebase is still for the yesterday behaviour

I was waiting for an OK about the new design. Code pushed 👍

[benoitf]

tested and works fine for me 🎉

[deboer-tim]

Much better, but for me it's a bit odd that you add the first one from a button on the left and then add more on the right (which 'moves' each time you add one), plus the fact that the initial button changes from 'add one' to 'remove all'. I'd suggest always keeping the "Add custom port mapping" button as-is, and each row just has a delete button on the right. Florent has already approved, so I'd be fine doing this as an additional PR.

[benoitf]

I'm fine to have follow up PRs or amend this one

[lstocchi]

Much better, but for me it's a bit odd that you add the first one from a button on the left and then add more on the right (which 'moves' each time you add one), plus the fact that the initial button changes from 'add one' to 'remove all'. I'd suggest always keeping the "Add custom port mapping" button as-is, and each row just has a delete button on the right. Florent has already approved, so I'd be fine doing this as an additional PR.

I see your point.

The initial idea was to have something similar to an Advanced settings button that makes visible a tab/panel with all extra settings. And then when clicking on it again the tab/panel gets closed. This is why the 'add one' and 'remove all'. For me it was more 'open the custom port mappings view', 'close it'.

I'm going to update this PR 👍

[lstocchi]

@deboer-tim something like this?

[benoitf]

@lstocchi would be nice to update the description body to be accurate ( the image)

[deboer-tim]

In case my thumbs-up isn't visible enough: yes! Thank you, looks great.

[lstocchi]

@lstocchi would be nice to update the description body to be accurate ( the image)

@benoitf You mean the image in the first message right?

[cdrage]

Tested and works well! Grats on the first ever svelte / UX/UI change merged PR! 🎉