Using bootstrap 4 and related UI refresh

[sappor0]

Hello, first of all sorry for the large MR, the changes were difficult to break down:

Changes at a glance:

• Switch from BS3 to BS4
• Polishing of some UI elements (low-hanging fruit for UX)
• Mobile-friendly layout.

Everything Nearly everything seems to work, there are some things I haven't managed to fix:

• The Content-Security-Policy prevents inline images from being used. This causes some of the custom form elements I used to not be displayed correctly. Not sure if this is only the case in dev builds.
• Some of the validation on-blur seems to be broken, this also causes test failures
• I've added a new string to the localisations (it's a label for the copy button for the croodle URL) – This needs to be translated for the other locales as this also causes failing tests.

… maybe someone who is a bit more versed in ember has some idea how to fix this.

Here are two screenshots:

Desktop

Mobile

[jelhan]

First of all: Thanks a lot for your amazing work. 🤗 Not only for fixing that technical debt (upgrade from Bootstrap 3 to 4) but also for the visual changes. Looks much better now. 👍

I haven't had the time to look into all details of UI changes but will do that the next days. But want to give you an early feedback with more technical stuff.

The CSP violations are caused by Bootstrap 4. It's a known issue: twbs/bootstrap#25394 Adding data: URL as a valid img-src to recommended CSP is the way to go in my opinion. You could fix the tests (and your development environment) by updating contentSecurityPolicy object in config/environment.js. The value for img-src should be something like "'self' data:". In quick tests I wasn't able to whitelist data URL used for SVG images (data:images/svg+xml) only. This is also described here: https://security.stackexchange.com/questions/94993/is-including-the-data-scheme-in-your-content-security-policy-safe/167244#167244

I think most of the tests will pass after fixing that CSP issue.

I noticed some other bugs on manual testing. I don't think all of them are related to the changes in this PR. Haven't had the time to test it on master yet. Will do that and move them to separate issues if they are present on master branch also.

• Validations are not shown if entering options ("I want to answer a question") and entering times ("I want to find a date").
• Validation error is not shown if user does not select any day ("I want to find a date").
• If user selects a day, clicks next button, clicks prev button and removes the only selected day, he's still able to go to times wizard page using top menu. The next button is disabled. ("I want to find a date") A user should not be able to open times selection page if no day is selected #208
• The box with the share link and the copy to clipboard button has a horizontal scrollbar (which is disabled). Tested in Chromium only.
• Validations are not shown on participation for answers ("Yes", "No", "Maybe"). Only present for name input.
• Validation state is flashing up for name input while participation is submitting. Validation state is flashing up for name input while participation is submitting #209

I've now idea why Tavis hasn't been run for this PR. Hope it will do as soon as you push additional commits.

[jelhan]

Closing and reopening to trigger Travis CI.

[jelhan]

Have fixed the Travis CI setup. Wasn't related to this PR but to a change I have done some while ago to Travis CI settings.

[jelhan]

Thanks for addressing the review feedback that quickly!

One acceptance test is failing due to broken validation:

not ok 14 Chrome 75.0 - [705 ms] - Acceptance | create a poll: create a poll for answering a question
    ---
        actual: >
            false
        expected: >
            true
        stack: >
                at _callee2$ (http://localhost:7357/assets/tests.js:247:24)
                at tryCatch (http://localhost:7357/assets/vendor.js:11833:40)
                at Generator.invoke [as _invoke] (http://localhost:7357/assets/vendor.js:12059:22)
                at Generator.prototype.<computed> [as next] (http://localhost:7357/assets/vendor.js:11885:21)
                at asyncGeneratorStep (http://localhost:7357/assets/tests.js:86:105)
                at _next (http://localhost:7357/assets/tests.js:88:196)
        message: >
            validation errors are shown after submit
        negative: >
            false
        browser log: |
            LOG: ember-i18n has been deprecated in favor of ember-intl
            WARN: WARNING: [ember-i18n-cp-validations] Missing translation for validation key: errors.validCollection
            http://offirgolan.github.io/ember-cp-validations/docs/messages/index.html

Also some component integration tests are failing for components {{create-options-datetime}} and {{create-options}} due to broken validations. But maybe it's only about adjusting test selectors.

If I didn't missed something only one test is failing that doesn't seem to be related to validations:

not ok 54 Chrome 75.0 - [98 ms] - Integration | Component | create options datetime: allows to delete an option

Let me know if you need any assistance for fixing that tests.

[sappor0]

Okay, so I get 6 failing tests locally, all of them are because of missing UI validation on the create options (datetime) screen.

I think this partially reflects the results of your manual testing but there appear to be some problems for which there are no tests.

• Validations are not shown if entering options ("I want to answer a question") and entering times ("I want to find a date").

• Validation error is not shown if user does not select any day ("I want to find a date").

These are the two issues which are detected as bugs by the tests I think.

• If user selects a day, clicks next button, clicks prev button and removes the only selected day, he's still able to go to times wizard page using top menu. The next button is disabled. ("I want to find a date")

Need a test for this. I can confirm that this is also the case on the BS3 version.

• The box with the share link and the copy to clipboard button has a horizontal scrollbar (which is disabled). Tested in Chromium only.

This was actually intentional. I didn't want to force-break the URL because this might make it invalid. At the same time I wanted to stop it from overflowing and breaking the grid. I assumed copy-pasting would be the use case 99% of the time.

But maybe we're talking about different things here. It looks like this to me:

• Validations are not shown on participation for answers ("Yes", "No", "Maybe"). Only present for name input.

• Validation state is flashing up for name input while participation is submitting.

Maybe this is related to the other validation issues.

[jelhan]

• If user selects a day, clicks next button, clicks prev button and removes the only selected day, he's still able to go to times wizard page using top menu. The next button is disabled. ("I want to find a date")
• Validation state is flashing up for name input while participation is submitting.

Both weren't related to this PR. Move them to separate issues #208 and #209.

[jelhan]

• The box with the share link and the copy to clipboard button has a horizontal scrollbar (which is disabled). Tested in Chromium only.

This was actually intentional. I didn't want to force-break the URL because this might make it invalid. At the same time I wanted to stop it from overflowing and breaking the grid. I assumed copy-pasting would be the use case 99% of the time.

But maybe we're talking about different things here.

The scrollbar was present but disabled cause not needed for my device. That got me even more confused. Makes more sense to me after seeing your screenshot. Thanks for including.

But I'm not quite sure if I got your point about why it's needed. The URL is broken in all cases if an additional character is added, isn't it? It doesn't make a difference if the user retypes it including a line break char after the question mark or at any other place. Selecting and copying it manually (not using the button) should work regardless if there is word-break: break-all or not. At least it has worked in my quick tests. 😇

[sappor0]

But I'm not quite sure if I got your point about why it's needed. The URL is broken in all cases if an additional character is added, isn't it? It doesn't make a difference if the user retypes it including a line break char after the question mark or at any other place. Selecting and copying it manually (not using the button) should work regardless if there is word-break: break-all or not. At least it has worked in my quick tests. innocent

Ah yes. I think I was worried that selecting and copying the text in the browser might insert whitespace if we let it break. But you're right: this wouldn't happen. I'll let it break normally so it is completely visible. Maybe we can make it autoselect the whole URL when the user clicks on it to prevent incomplete selections.

[sappor0]

… I've just pushed a change which makes it look like this. I've removed the link, instead if a user clicks on it the whole text is selected which feels more correct to me. What do you think?

[sappor0]

Maybe you could take a look at why the validations are not happening in some cases. I'm stuck 😒

[jelhan]

This also fixes #112, isn't it?

… I've just pushed a change which makes it look like this. I've removed the link, instead if a user clicks on it the whole text is selected which feels more correct to me. What do you think?

Awesome! 👏

Maybe you could take a look at why the validations are not happening in some cases. I'm stuck 😒

Will have a look as soon as possible.

[jelhan]

Have had a more detailed look on the design and have some small feedback.

1. Bar indicating wizard steps

Is there any reason that the top bar indicating the steps in wizard isn't using full width on desktop?

It's currently looking like this for my default screen size:

This is how full width looks like. Have done it by setting width: 100% and text-align: center on @media (min-width: 768px) .cr-steps-top-nav:

2. +-Button in forms

The +-Button is not inline with the input and the Delete-Button in the form for setting times (find a date) and editing options (make a poll):

Is there any reason that both button aren't in the same line as the input? If it is, it's looking like this:

Have manually moved the button into the .input-group-append to take that screenshot.

[sappor0]

1. **Bar indicating wizard steps**

I tried it like this initially but I found the different spaces between the menu item labels make it feel a little unstructured. I think in the next iteration this would be one of the things I would like to change completely anyway (into a proper stepper, maybe vertically aligned on the side on desktop view ports) so I didn't want to spend so much time on it.

1. **`+`-Button in forms**

The deletion of an item (the minus) is specific to that item, while the plus actually has nothing to do with the item itself but with the position the item will be added into (i.e. between two items) – that's why it's not part of the contextual item menu. (I think I would add other things there in future, maybe a drag and drop handle for instance)

I also recently tried out doodle and discovered a very nice feature of theirs: you always have an empty field at the bottom, if you add content to it another empty field is added so you always have enough fields and you never need to manually add fields. – I was thinking that maybe this functionality with drag and drop handles would provide a pretty neat and clean UI. (I would argue that adding an item between two other items is an edge case and it can be achieved by adding and then repositioning with drag and drop anyway)

[jelhan]

Thanks for your detailed explanation. Makes sense to me.

I thinks this is ready to be merged after you have merged in sappor0#1. Is there anything outstanding from your side?

[sappor0]

Nope. AFAIK it's ready and all the tests are passing when I run it locally 👍

Thanks for the fixes!

[jelhan]

Merged. 🎉 Thanks a lot for your awesome work in this PR! 👏