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

Updated popover rules to allow form inputs and added input submit for… #8575

Merged
merged 13 commits into from
Mar 8, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Feb 28, 2018

… new File
Fixes #1904

capture d ecran_2018-03-06_10-30-30

Also new standard for the popovermenu

<div class="popovermenu">
    <ul>
        <li>
            <a href="#" class="menuitem active">
                <span class="icon icon-folder"></span>
                <input id="input-folder" type="text" value="New folder">
            </a>
        </li>
        <li>
            <a href="#" class="menuitem active">
                <span class="icon icon-folder"></span>
                <input id="input-folder" type="text" value="New folder">
                <input type="submit" value=" " class="primary icon-checkmark-white">
            </a>
        </li>
        <li>
            <a href="#" class="menuitem active">
                <span class="icon icon-folder"></span>
                <input id="input-folder" type="text" value="New folder">
                <input type="submit" value=" " class="primary icon-checkmark-white">
                <input type="reset" value=" " class="icon-close">
            </a>
        </li>
        <li>
            <a href="#" class="menuitem active">
                <span class="icon icon-folder"></span>
                <form>
                        <input id="input-folder" type="text" value="New folder">
                        <input type="submit" value=" " class="primary icon-checkmark-white">
                </form>
            </a>
        </li>
    </ul>
</div>

@skjnldsv skjnldsv added design Design, UI, UX, etc. 3. to review Waiting for reviews low papercut Annoying recurring issue with possibly simple fix. standardisation labels Feb 28, 2018
@skjnldsv skjnldsv added this to the Nextcloud 14 milestone Feb 28, 2018
@skjnldsv skjnldsv self-assigned this Feb 28, 2018
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Instead of a separate button, it should just be an icon-confirm element on the right inside the input field. :)

Just like we do in the "Add to your Nextcloud" on the sharing page.

@skjnldsv
Copy link
Member Author

Oh yes it could!! :)

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 28, 2018
@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 5, 2018

Okay, I tried to add the standard I was talking you about @jancborchardt :)
The only downside I found is the focus on the confirm button itself which is impossible to get rid of since we can't select previous sibling in css. Nonetheless I don't find it very problematic!

@nextcloud/designers

capture d ecran_2018-03-05_17-17-31

@skjnldsv skjnldsv force-pushed the new-folder-button branch 4 times, most recently from bfc95d5 to a8f2fe0 Compare March 5, 2018 16:49
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 5, 2018
@juliusknorr
Copy link
Member

Looks nice @skjnldsv I'd just say the icon confirm button should have a default opacity, to match the text color of the form field.

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 6, 2018
@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 6, 2018

@juliushaertl Yes, I'm fixing all of this :)
There was indeed some unwanted behavior ;)

@codecov
Copy link

codecov bot commented Mar 6, 2018

Codecov Report

Merging #8575 into master will decrease coverage by 0.48%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #8575      +/-   ##
============================================
- Coverage     51.87%   51.38%   -0.49%     
- Complexity    25367    25368       +1     
============================================
  Files          1607     1541      -66     
  Lines         95154    87340    -7814     
  Branches       1379        0    -1379     
============================================
- Hits          49362    44883    -4479     
+ Misses        45792    42457    -3335
Impacted Files Coverage Δ Complexity Δ
...s_sharing/lib/Template/ExternalShareMenuAction.php 25% <0%> (-6.25%) 2 <0> (ø)
lib/private/Template/JSCombiner.php 87.5% <0%> (-1.61%) 31% <0%> (+1%)
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
...mments/js/vendor/At.js/dist/js/jquery.atwho.min.js
core/js/js.js
core/js/octemplate.js
core/js/sharedialogresharerinfoview.js
apps/files_sharing/js/sharetabview.js
apps/files_versions/js/versionstabview.js
core/js/sharesocialmanager.js
... and 58 more

@skjnldsv skjnldsv added the 3. to review Waiting for reviews label Mar 6, 2018
skjnldsv added 3 commits March 7, 2018 13:17
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the new-folder-button branch from caaa1b5 to e2910e1 Compare March 7, 2018 12:17
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 7, 2018
@MorrisJobke
Copy link
Member

@MorrisJobke there is only the bottom margin to fix, but it's kind of annoying because it does looks good on inputs, but not on texts popovers:

In the first image it looks at least better than before. The last image you posted the little folder icon and the text in the last input are not aligned anymore. Could you revert to #8575 (comment) ?

@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 7, 2018

@MorrisJobke yes, there's a fix incoming! :)

EDIT: okay, still not some fixes

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 7, 2018

Okay alignment is good now!
@MorrisJobke please review :)

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 7, 2018
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Looks a lot better now 👍

bildschirmfoto 2018-03-07 um 15 21 21

bildschirmfoto 2018-03-07 um 15 21 00

@MorrisJobke
Copy link
Member

MorrisJobke commented Mar 7, 2018

Just noticed one issue:

  • clicking that button causes a page reload in Safari, @skjnldsv does this also happen in your instance? I couldn't reproduce in Chrome :/

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 7, 2018

@MorrisJobke oups forgot about that! Sorry, only the new folder button?

I don't have this behavior on chrome, let me check

edit: nope, and I did not changed the bahavior, the submit function is still executed with a stopPropagation :/

event.stopPropagation();
event.preventDefault();

@MorrisJobke
Copy link
Member

@MorrisJobke oups forgot about that! Sorry, only the new folder button?

New folder and new file.

@MorrisJobke
Copy link
Member

I don't have this behavior on chrome, let me check

Me neither - only in Safari :/

@MorrisJobke
Copy link
Member

clicking that button causes a page reload in Safari, @skjnldsv does this also happen in your instance? I couldn't reproduce in Chrome :/

Also pressing enter causes a reload of the page in Safari (nevertheless master and this branch work totally fine in Chrome :()

@@ -171,6 +172,10 @@
}
});

$submit.click(function(){
$form.submit();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe stop here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you try again with the last commit? @MorrisJobke :)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@MorrisJobke
Copy link
Member

clicking that button causes a page reload in Safari, @skjnldsv does this also happen in your instance? I couldn't reproduce in Chrome :/

Tested and works now 👍

@rullzer
Copy link
Member

rullzer commented Mar 8, 2018

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 8, 2018

@rullzer js tests passed

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

lets do this

@rullzer rullzer merged commit 7e37420 into master Mar 8, 2018
@rullzer rullzer deleted the new-folder-button branch March 8, 2018 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. low papercut Annoying recurring issue with possibly simple fix. standardisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New folder: icon-confirm button for mouse users
5 participants