Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Add "Open in new window" option to links #2628
Conversation
notnownikki
added
the
Needs Design
label
Aug 31, 2017
codecov
bot
commented
Aug 31, 2017
•
Codecov Report
@@ Coverage Diff @@
## master #2628 +/- ##
==========================================
+ Coverage 31.42% 31.56% +0.14%
==========================================
Files 177 178 +1
Lines 5407 5613 +206
Branches 946 988 +42
==========================================
+ Hits 1699 1772 +73
- Misses 3135 3233 +98
- Partials 573 608 +35
Continue to review full report at Codecov.
|
| @@ -130,6 +146,13 @@ class FormatToolbar extends Component { | ||
| isActive: !! formats[ control.format ], | ||
| } ) ); | ||
| + // TODO: make this not look hideous | ||
| + const linkSettings = settingsVisible && ( |
notnownikki
Aug 31, 2017
Collaborator
Nope, mainly because I didn't know what it was
notnownikki
Sep 1, 2017
Collaborator
I've switched this to use a popover, but it still looks really bad. I'm not sure what markup we need to have this render like the wireframes, and there's nowhere that we're actually using a PopoverContext for me to see how to get this rendering nicely, either. Also, there are focus issues once you toggle the checkbox (click the settings icon, toggle the checkbox, then you have to click the settings icon twice to close the Popover, as I think the first click takes the focus away from the Popover, which feels bad.)
notnownikki
added some commits
Sep 1, 2017
|
This works now, and is using the correct form components, but for the life of me I can't get the settings to appear below the toolbar, they just squish everything up. Perhaps @karmatosed can work some magic here, or advise me? |
|
Tweaked the styling a bit:
|
notnownikki
added
[Component] Editable
and removed
Needs Design
labels
Sep 1, 2017
|
I've set
Perhaps @afercia could make sure I've got this right? Happy to squash and merge if everything looks ok :) |
youknowriad
reviewed
Sep 4, 2017
Should we leave the settings visible when we toggle the target? They are being closed in my testing
| @@ -93,6 +99,17 @@ class FormatToolbar extends Component { | ||
| }; | ||
| } | ||
| + toggleLinkSettingsVisibility() { | ||
| + const settingsVisible = ! this.state.settingsVisible; | ||
| + this.setState( { settingsVisible } ); |
youknowriad
Sep 4, 2017
Collaborator
there's an implicit coding standard stating that if the next state depends on the previous, we should use the callback notation (I think we were trying to add an eslint for this in an old PR)
this.setState( ( state ) => ( { settingsVisible: ! state.settingsVisible } ) );
|
Am I the only one seeing this design bug
|
|
@youknowriad I'm not seeing either of those things happen... what browser/OS? |
|
(that is, setting stay open when I toggle the setting, and the toolbar doesn't jump to the next line) |
|
Weird!!! MacOS Chrome Version 60.0.3112.113. I reproduce in Firefox too 55.0.3 (64 bits) |
|
I see the setting icon wrap onto the next line in firefox, but I can't reproduce the "settings close when setting is toggled" thing in any browser. I'll have to defer to someone else to fix the wrapping bug though, still not sure how to get that working correctly |
|
Hmm, perhaps there's a click event we need to stop bubbling up somewhere? I can't see where that would be though, the settings fieldset isn't inside the IconButton... |
|
The grumpy z-index monster is back:
This only happens when the screen size is reduced. |
|
After testing I have found the same FF issue:
However, I can't replicate the other one. Design wise I think if we can work out those issues, I would love to get this into 1.1. I am not sure this is perfect, but I would like to get user feedback on it and that's a great way to do it. |
|
I'm not entirely sure how to fix the FF wrapping issue... if anyone has a bit of spare time, I'd be happy to learn from them :) |
notnownikki
added some commits
Sep 5, 2017
|
I think I've addressed the issues here, it renders fine in chrome and firefox now, although I can't reproduce the issue where the settings close when you toggle a setting, and I can't see the design issue @youknowriad saw where all the link icons where wrapped onto the next line, no matter what browser I use. |
|
Looks good to me, so going to merge. |
notnownikki commentedAug 31, 2017
•
edited
Fixes #2204