Skip to content

Conversation

VaZark
Copy link
Contributor

@VaZark VaZark commented Nov 23, 2022

Related to #209

These are purely visual changes based on a boolean field.

Significant changes are,

  • Two new fields show_fieldsets_as_tabs & show_inlines_as_tabs and it's migration.
  • headless_* version of fieldsets and inlines templates in order to hide the header. They are exact copies except for the title line.
  • Template tag to fetch headerless_* version of fieldset and inlines when in tabbed mode
  • JS file with basic tab logic
  • Static CSS for tab buttons

@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Base: 95.58% // Head: 95.70% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (18e63db) compared to base (18f865b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
+ Coverage   95.58%   95.70%   +0.11%     
==========================================
  Files          36       37       +1     
  Lines         408      419      +11     
==========================================
+ Hits          390      401      +11     
  Misses         18       18              
Flag Coverage Δ
unittests 95.70% <100.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
admin_interface/admin.py 100.00% <ø> (ø)
...ions/0028_theme_show_fieldsets_as_tabs_and_more.py 100.00% <100.00%> (ø)
admin_interface/models.py 100.00% <100.00%> (ø)
...min_interface/templatetags/admin_interface_tags.py 85.00% <100.00%> (+1.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fabiocaccamo
Copy link
Owner

@VaZark thank you for this PR, I will test and review it as soon as possible.

lint step failed because you added some translatable strings, but you have not updated translations files running tox -e translations (I should update the README).

I also updated the .tox config file on the main branch.

Copy link
Owner

@fabiocaccamo fabiocaccamo left a comment

Choose a reason for hiding this comment

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

@VaZark
Copy link
Contributor Author

VaZark commented Nov 24, 2022

Regarding tabs, a quick look through the CSS shows that there are assumptions about the structure of the tab.

For example in jquery.ui.tabs.css, we have this line

.admin-interface .ui-tabs .ui-tabs-nav li a {

So it assumes the items are hyperlinks within a list. This is not at all in line with my implementation, which is just a bunch of buttons with flex and wrap. I don't there is a way I can reliably reuse them. :(

I'll give it shot tho

@fabiocaccamo
Copy link
Owner

Regarding tabs, you can't reuse that CSS, but you can replicate the same design of django-tabbed-admin with CSS customizations.

@VaZark
Copy link
Contributor Author

VaZark commented Nov 24, 2022

I have not used django-tabbed-admin and the image link on the project page is broken. Can you share a screenshot if u have one ? I'm lost on which CSS customisations that I am missing.

fabiocaccamo
fabiocaccamo previously approved these changes Nov 24, 2022
@fabiocaccamo
Copy link
Owner

This is how django-admin-interface + django-tabbed-admin looks:

Screenshot 2022-11-24 at 15 23 02


{% if show_fieldsets_as_tabs %}
{% for fieldset in adminform %}
<button type="button" class="tabbed-changeform-tablinks {{ forloop.counter0|default:"active" }}" onclick="openTab(event, '{{fieldset.name}}')">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any appetite for making the tabs work purely with CSS?

https://www.sitepoint.com/css3-tabs-using-target-selector/

Copy link
Owner

Choose a reason for hiding this comment

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

Avoiding JavaScript at all for this feature would be great!

Copy link
Contributor Author

@VaZark VaZark Nov 25, 2022

Choose a reason for hiding this comment

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

I find that using JS is the most readable way to handle the logic.

Most CSS-only solutions I've come across are not easy to implement to be responsive and handle a dynamic no of tabs 😓

You're welcome to change it up though. 👍

Copy link
Owner

Choose a reason for hiding this comment

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

Personally I have not any experience with CSS only tabs, I can't evaluate implementation complexity.

@VaZark
Copy link
Contributor Author

VaZark commented Nov 25, 2022

Current state of the tabs
image

@fabiocaccamo
Copy link
Owner

Cool the tabs style, I would just make the border of a neutral color (medium grey).

@VaZark
Copy link
Contributor Author

VaZark commented Nov 25, 2022

Used the --border-color css var

@fabiocaccamo
Copy link
Owner

Does the horizontal scroll already work?

@VaZark
Copy link
Contributor Author

VaZark commented Nov 25, 2022

Yup.

@fabiocaccamo
Copy link
Owner

Could you update the translations files?

python -m django makemessages --all --add-location=file
python -m django compilemessages --ignore ".tox" --ignore ".venv" --ignore "build" --ignore "venv"

@fabiocaccamo fabiocaccamo merged commit cc1f72b into fabiocaccamo:master Nov 26, 2022
@VaZark
Copy link
Contributor Author

VaZark commented Nov 26, 2022

Thanks for updating the translations :)

@VaZark VaZark deleted the tabbed-changeform branch November 26, 2022 12:31
@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Nov 28, 2022

@VaZark I finally tested this feature in a real project and I noticed that there are some issues to fix and some improvements needed before releasing it:

  • After installation, I noticed that "inlines as tabs" option is set because its default value is True, it should be False (sorry I missed it during the review).

  • When "inlines as tabs is set", there is a tab even if the model admin has not inlines, in this case there should be not any tab (if the sum of all tabs is one, don't show tabs at all).

  • On page load the tab has an unexpected border/outline visible.

Screenshot 2022-11-28 at 18 00 38

  • When there are many tabs and there is horizontal overflow, the scrollbar overlaps the tabs (bad UX), it should be placed below the bordered bottom line.

  • When "saving and continue editing" the selected tabs is lost, should click it again. This should be solved using a slugified URL fragment of the selected tab (to check on page load and update on tab change), eg. http://localhost:8000/en-us/admin/admin_interface/theme/1/change/#form-controls.

I tested it with Chrome and Firefox.

@VaZark
Copy link
Contributor Author

VaZark commented Nov 28, 2022

Thanks. I’ll take a look tmrw

@merwok
Copy link
Contributor

merwok commented Dec 6, 2022

One difference between collapsed fieldsets and tabbed fieldsets is that it’s not possible anymore to go to the next section using Tab. Could we add tabindex attributes?

@fabiocaccamo
Copy link
Owner

@VaZark can you handle the tabindex problem?

@fabiocaccamo fabiocaccamo added the enhancement New feature or request label Dec 7, 2022
@VaZark
Copy link
Contributor Author

VaZark commented Dec 7, 2022

if i'm reading this right, the goal is to open and move to the next tab instead of the save/submit section when we are the end of a fieldset on clicking the "tab" button?

It currently does not work as the tabs are hidden with display:hidden and not part of the DOM generated, so just adding a tabindex should not fix the problem.

However, if the goal is to just switch section from tab header using "tab" button, it still works. we just need to add a visual cue

@merwok
Copy link
Contributor

merwok commented Dec 7, 2022

I didn’t meant the sections, which are hidden as you say, but the tabs themselves, the little rectangles with labels which are javascript links.

@VaZark
Copy link
Contributor Author

VaZark commented Dec 9, 2022

If this is what is expected, I can send in the PR in a sec

Screencast.from.2022-12-09.10-16-37.webm

@merwok
Copy link
Contributor

merwok commented Dec 9, 2022

Does this mean:

  • Tab goes through the tabs, then the form controls of the currently visible section
  • if you click inside the form, Tab goes to the next form control

I think it would be good then, although I’m unsure if it’s best to go through tabs then form controls, or form controls then tabs.

@VaZark
Copy link
Contributor Author

VaZark commented Dec 9, 2022

Conventional fieldsets are just dividers that contain text in a continuous form, tabs are closer to sub-pages.

Even github scrolls through the tab headers before stepping into the tab content. Jumping back to the top of the page when tabbing through is not a nice experience.

@merwok
Copy link
Contributor

merwok commented Dec 9, 2022

Sorry, could you reply yes/no to my two points so that I understand the behaviour?

@VaZark
Copy link
Contributor Author

VaZark commented Dec 10, 2022

It’s yes and yes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants