There is non sense to leave a lot of spaces to the Operations columns. especially dropdown menu introduced into CORE, most of Operations columns are SINGLE button only.
Beta phase evaluation
Issue category | Task since the UI is not broken. |
---|---|
Issue priority | Normal since this is an improvement (not just a code cosmetic change which would be minor). |
Unfrozen changes | Unfrozen because it only changes CSS. |
Allowed in the beta since it is an unfrozen change: just CSS.
Before:
After:
Remaining Tasks
- make the big patch to affect all tables
- Update code that tests the output of the tables
Original Issue summary
It will be a big patch, would like to hear feedbacks before I made it into all tables. Attached a quick patch for tests. :)
It used same technique as #1252206: Remove checkbox spacing
Comment | File | Size | Author |
---|---|---|---|
#84 | claro-db-log.png | 299.34 KB | AkshayAdhav |
#84 | claro-people.png | 230.07 KB | AkshayAdhav |
#84 | seven-db-log.png | 360.43 KB | AkshayAdhav |
#84 | seven-people.png | 241.69 KB | AkshayAdhav |
#82 | 1849750 query.png | 3.83 KB | aarti zikre |
Issue fork drupal-1849750
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
droplet CreditAttribution: droplet commentedminimize.patch queued for re-testing.
Comment #3
YesCT CreditAttribution: YesCT commentedDrupal\system\Tests\Menu\RouterTest is probably and unrelated random failure in the testbot. retesting.
Comment #4
YesCT CreditAttribution: YesCT commentedminimize.patch queued for re-testing.
Comment #5
mitron CreditAttribution: mitron commentedWhy are javascript changes included in this patch? Are these change required/related to minimizing the width of the operations column?
Comment #6
samhassell CreditAttribution: samhassell commentedCant see how code in autocomplete.js would affect this. The CSS change looks good though.
Here's a reroll without the js.
Comment #7
YesCT CreditAttribution: YesCT commenteda screenshot of a dropbutton with a long action word would be helpful.
Does this take into account the width of items that are under the dropbutton, but not the default first item?
(restoring Usability tag)
Comment #8
droplet CreditAttribution: droplet commentedOh sorry. I attached a wrong patch with other fixes.
only this line is good enough
Comment #9
droplet CreditAttribution: droplet commentedadd back the tag
Comment #10
YesCT CreditAttribution: YesCT commentedhere is a long action word.
this just effects views, was that intentional, or should it apply to really *all* operations columns?
I was looking at content listing, but that will become a view... but I think there are others that will not be views,
... like the translations tab/overview on content.
Comment #11
YesCT CreditAttribution: YesCT commentedOh, sorry. Ack! It totally says that this will be a big patch and the initial patch is just an initial patch for quick test.
So I can answer my own question. This may eventually, change *all* operations columns.
Comment #12
droplet CreditAttribution: droplet commentedIt's happening without this patch. anyone able to create a new issue.
Comment #13
YesCT CreditAttribution: YesCT commentedhere is the separate issue for long text #1914800: Dropbutton width is smaller than longest item
Comment #14
YesCT CreditAttribution: YesCT commentedsetting to needs work to make this effect all operations columns.
updating issue summary.
Comment #14.0
YesCT CreditAttribution: YesCT commentedupdated to add a remaining tasks and followup issues section. also to make clear that next is to make the big patch.
Comment #15
enhdless CreditAttribution: enhdless commentedComment #16
klonos...related issues now have their dedicated meta-data section ;)
Comment #17
nsur CreditAttribution: nsur commentedWe tested various tables in Drupal 8 and looks like the column has an optimal size as its contents may vary depending on the language interface. See attached screenshots. We think that no changes are necessary, unless we missunderstood the issue.
Comment #18
droplet CreditAttribution: droplet commented@nsur,
Aim to make it more optimal size. You can see the Operations column on your screenshots, it still leave a lot of space on the right side. the code we use will be fluid width based on the column header. In other word, it still has an optimal size on every language interface.
any UX maintainers here ? can you share your thoughts.
( More deeply, I think it should apply same techniques to all less important columns. E.g., Status, Author, Tags...etc. )
Comment #19
LewisNyman CreditAttribution: LewisNyman commentedThis patch is changing code in the views modules, much better to get a decision from the VDC team.
Comment #20
LewisNyman CreditAttribution: LewisNyman commentedComment #21
ChuChuNaKu CreditAttribution: ChuChuNaKu commentedHello, I'm a new contributor here at the Austin, TX Code Sprint. I will be working on this issue.
Comment #22
ChuChuNaKu CreditAttribution: ChuChuNaKu commentedComment #23
ChuChuNaKu CreditAttribution: ChuChuNaKu commentedWhile testing #6 I received the following error:
I think the cause of the error is because the views-admin.theme.css file has been moved to core/modules/views_ui/css/views-admin.theme.css. I made a new patch that includes the new css location.
Comment #24
droplet CreditAttribution: droplet commentedThanks @ChuChuNaKu.
I removed #novice tag here. We need a consensus on this topic first.
Comment #25
ChuChuNaKu CreditAttribution: ChuChuNaKu commentedCreated a new patch which should shrink the widths of all operations columns (not just the operations column in views). This patch assumes the following:
1. the operations column is the last column in the table
2. the operations column is wrapped in a .tableresponsive-processed class
Comment #26
jackbravo CreditAttribution: jackbravo commentedHi ChuChuNaKu! Great meeting you in Austin.
So, a couple of notes on your patch. Drupal is supposed to work with IE8, the requirements page is here: https://drupal.org/requirements, and from there you can go to browser requirements. :last-of-type does not work on IE8.
I'm not sure if maybe we should look for a way to add a specific class for the operation columns that are not using views. Both menu and taxonomy use a _entity_list kind of thingy that I'm not entirely sure right now how to modify. But I'm looking into it. You can also try looking something like that.
Your other patch works good for me.
Comment #27
jackbravo CreditAttribution: jackbravo commentedWow!
Tracking where the operations column was defined was hard! Probably it would have been easier to just search for "Vocabulary name" (the header column of the taxonomy vocabulary listing) and go from there.
Anyway, this is an expanded patch based on ChuChuNaKu. Maybe we could try moving all the CSS to something like core/modules/system/css/system.admin.css?
Comment #29
ChuChuNaKu CreditAttribution: ChuChuNaKu commentedjackbravo it was great meeting you too and thanks so much for your help at the Austin Sprint. I really appreciate it!
Thanks for bringing the browser requirements to my attention. I think adding a class is the right way to go. I will look into it a little further.
Comment #30
jackbravo CreditAttribution: jackbravo commentedSince the last patch changed the buildHeader output for must operations, the code that tests that output needs to be changed too. That may be a novice remaining task. Updating the code in core/modules/config/src/Tests/ConfigEntityListTest.php that tests the buildHeader output. Starting on line 73.
Comment #31
jackbravo CreditAttribution: jackbravo commentedsorry, wrong tag =(
Comment #32
droplet CreditAttribution: droplet commentedalso, some wrong class,
view's class: .views-field-dropbutton
Comment #33
YesCT CreditAttribution: YesCT commenteddoing https://www.drupal.org/contributor-tasks/update-allowed-beta
to evaluate https://www.drupal.org/contribute/core/beta-changes
Comment #34
YesCT CreditAttribution: YesCT commentedunassigning since it has been a while. @ChuChuNaKu please feel free to jump back in, just make a comment.
Comment #35
BarisW CreditAttribution: BarisW commentedHere's a re-roll. Screenshots from #25 still apply.
Comment #36
DomoSapiens CreditAttribution: DomoSapiens commentedLong words in Dutch mess up the layout. Because of the position:absolute dropdown the size isn't correct and will not grow on the proper way.
Comment #37
DomoSapiens CreditAttribution: DomoSapiens commentedI tried to fix the styling of the dropdown. In my opinion there are two solutions.
1) The simple solution is to position the dropdown to the right of the td. In this case the dropdown is or can be bigger as the td. With a lot text / items in one row the dropdown will overflow the other items. Which is not really a nice approach. For this easy solution I added a patch.
2) The more complex solution is to rewrite the html structure of the dropdown. In the new html structure the secondary items should be in a separate container. The first item and the toggle can go without the position absolute, which make the td grow. The secondary items can have the position absolute instead, which make it possible that the dropdown will overlay the other rows.
Comment #38
corbacho CreditAttribution: corbacho commentedIt's strange that the dropbutton is not aligned with the "Operations" label (Table header)
RTL needs to be considered
In wide screens (most of laptops), and 2 columns table, it feels strange to have so much white space in the center of the table. See screenshot.
Maybe we should consider target only the problematic cases.
Comment #39
droplet CreditAttribution: droplet commentedPlease test patch #35
@DomoSapiens,
see: #1890266: dropbutton text fails to retain .dropbutton-widget width
Comment #40
corbacho CreditAttribution: corbacho commentedThe patch at #35 is taking too much assumptions. It affects the last column of every single table, and things look smashed when there are long texts.
At #17:
(during a sprint, so it seems several people was involved here)
I'm on the same page.
The whitespace in the right of the last column disappears in narrower screens. So there is no problem in mobile/tablet.
The default table layout already makes a good work keeping balanced whitespace, at all screen sizes
If there is a problematic case, (Operations column) let's focus on that, like patch at #25 and #27, but IMO I don't think we should touch generic component tables.css. We will probably do more harm than good.
Comment #41
Ramandeep_Kaur CreditAttribution: Ramandeep_Kaur commentedComment #43
joginderpc CreditAttribution: joginderpc at Srijan | A Material+ Company commentedAs per given scenarios given in both comments #38 and #40 issue fixed and update with screenshot given below.
Comment #45
joginderpc CreditAttribution: joginderpc at Srijan | A Material+ Company commentedRe-rolled Patch.
Comment #46
joginderpc CreditAttribution: joginderpc at Srijan | A Material+ Company commentedComment #48
joginderpc CreditAttribution: joginderpc at Srijan | A Material+ Company commentedAdd new patch re-rolled, finger crossed this time it should pass. :)
Comment #49
Amruta_Nadgouda CreditAttribution: Amruta_Nadgouda as a volunteer and commentedBug : Now the opration column's width is set right as shown in description.
Comment #50
xjmThanks for working on this issue.
In general, we do not leave commented-out code in patches. If this is incorrect, it should be removed.
Have we tested this on both RTL and LTR? (RTL means a language that goes from right to left, like Hebrew or Arabic.) Edit: and if so, please document both in the summary.
I would remove the words "added to" from this, and capitalize the beginning of the comment.
Targeting this issue for 8.3.x as a design improvement. Thanks!
Comment #51
Manjit.Singh@Joginderpc Personally, I did not like the solution to handle operations table header. I have take some of the screenshots where it is causing an issues. Because your CSS is applied to the very last column of all tables.
There are some other tables also which are effecting.
Comment #52
Gábor HojtsyYeah it does seem like an inaccurate generalization to assume that all tables displayed in the Seven theme would need their last column shortened. We need to target this CSS better.
Comment #53
jackbravo CreditAttribution: jackbravo commentedWhy was the idea on the patch on comment #1849750-27: Minimize all "Operations" columns width (my patch :-p) no followed up? The idea was to put a class on the operations column, so it would be easier adding styles to it without either too much generalization or too complicated CSS.
This is the patch:
Comment #58
PanchoAnother related bug in Views.
Comment #62
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedPatch #48 failed to apply on 9.1.x.
Review the patch attached screenshot for the reference.
Comment #64
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedFixing test cases.
Review the patch attached screenshot for the reference.
Comment #66
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedTested #64, columns width are successfully minimized
Comment #67
djsagar CreditAttribution: djsagar at OpenSense Labs commentedTested #64, columns width minimized successfully after applied patch.
Comment #68
alexpottCoding standards tests are failing.
Comment #69
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
Creating a patch for solving an issue raised due to tests are failing.
Please review the patch.
Thanks.
Comment #70
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #71
alexpottselect-all
is specifically used by the tableselect javascript to add a select all checkbox. Re-using that here feels incorrect because this has nothing to do with selecting all. I think we should consider other solutions - maybe adding a new class. Also if a translation has translatedOperations
into more than one word this change might have the affect of causing a line break in the header whereas before there wouldn't be. I'm not sure if this is a good or bad thing but it is certainly a change.Comment #73
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commented@Pooja Ganjage Thanks for the patch patch applied successfully and looks good for me
After patch unwanted space has been remove working in dupal-9.3.x-dev
Thanks for the patch
For ref sharing screenshots.....
Comment #78
AkshayAdhavPlease review the MR.
This MR includes all the tables which have operations column.
Pages checked:
Block layout
/admin/structure/block
Comment types
/admin/structure/comment
Contact forms
/admin/structure/contact
Content Types
/admin/structure/types
Manage fields
/admin/structure/types/manage/article/fields
Form modes
/admin/structure/display-modes/form
View modes
/admin/structure/display-modes/view
Menus
/admin/structure/menu
Taxonomy
admin/structure/taxonomy
Text formats and editors
/admin/config/content/formats
Search pages
/admin/config/search/pages
Image styles
/admin/config/media/image-styles
Date and time formats
/admin/config/regional/date-time
Comment #79
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedReviewing this
Comment #80
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #81
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #82
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedVerified the merge request on drupal 9.5.x version.
https://git.drupalcode.org/project/drupal/-/merge_requests/2523/diffs
Testing Steps:
* Create new instance of drupal 9.5.
* Added & fetched this issue fork’s repository by below commands
git remote add drupal-1849750 git@git.drupal.org:issue/drupal-1849750.git
git fetch drupal-1849750
* git checkout
git checkout 1849750-minimize-all-operations
* Clear all the caches
Visited pages: (Seven)
Claro
Same thing done for it.
How ever can we reduce the size of this drop-down to?
This needs works
Comment #83
aarti zikre CreditAttribution: aarti zikre as a volunteer and at QED42 commentedComment #84
AkshayAdhav@aarti I have worked on taxonomy terms listing page, where it was required to make the changes.
But I think we don't need to do any changes on People listing page or DB log page. As there is only single operation button and spacing seems sufficient. Also these are views pages, so we will need to add classes in respective views config.
Please check the uploaded screenshots.
@alexpott - your views on it please.