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

Fix filename column width in responsive mode #2999

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Feb 11, 2020

Description

Removed uk-width-small and now using the icon width for column
alignment. This gives more space in responsive mode for the file name
column.
Removed the "More" column header label which felt redundant.
Adjusted file actions button id to not mention size any more, now that
we only have a single size.

  • Removed uk-width-small and now using the icon width for column
    alignment. This gives more space in responsive mode for the file name
    column.
  • Removed the "More" column header label which felt redundant.
  • Adjusted file actions button id to not mention size any more, now that
    we only have a single size.
  • Share indicator column now always present even when indicators are not
    rendered, for consistency with the header.
  • On resize, copy the table body row width to the header row to make sure
    that headers stay aligned when a scrollbar appears in the body as
    the scrollbar does not cover the header part as the latter is
    fixed/frozen on scroll.

Related Issue

Fixes #2998

Motivation and Context

How Has This Been Tested?

  • manual test in responsive and regular mode to check file name column and also the column header alignment with column contents

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

very nice!

@PVince81 PVince81 force-pushed the fix-responsive-filename-width branch from dac040b to 4d111c9 Compare February 12, 2020 08:13
@PVince81 PVince81 requested a review from kulmann February 12, 2020 08:13
@PVince81
Copy link
Contributor Author

@kulmann please recheck all, I've also added "flip: false" now to the new file menu after rebasing

kulmann
kulmann previously approved these changes Feb 12, 2020
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

🚀

}
}
}
</script>
<style scoped>
/* FIXME: move to ODS, it is useful to have a column as wide as just an icon */
Copy link
Member

Choose a reason for hiding this comment

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

When you are raising a ticket for this, add icon size classes as an idea. So oc-icon-width with the default icon width, then oc-icon-width-xsmall, oc-icon-width-medium, etc.

@PVince81 PVince81 force-pushed the fix-responsive-filename-width branch from 4d111c9 to 9f6db0e Compare February 12, 2020 19:27
@PVince81
Copy link
Contributor Author

Indeed I can just set "oc-icon" on the header div to get the width, done that now.

After rebasing on top of master and getting #1952, I have now a bigger alignment problem:
the header bar of the table has padding-small, but has no scroll bar. So we're missing some additional space on the right

So now I'd need to set the column width to 39px instead of just 24px to accommodate for that. The 24px only works fine when no scrollbar is here. I don't think I can detect the scrollbar width, and it would be hacky anyway.

With scrollbar:
image

Without scrollbar:
image

Any suggestions ? Would it be possible to have a fixed table header but still have the scrollbar pushing the layout ? @LukasHirt

@PVince81 PVince81 dismissed kulmann’s stale review February 12, 2020 19:29

Conditions have changed after rebase

@PVince81
Copy link
Contributor Author

Idea: use JS to measure the width of the name column in the body (it's the one that expands as far as possible) and then apply the same with to the name column in the header. This would help align the column headers and is likely easier than trying to guess the scrollbar size.

Otherwise I'd wish for a way with CSS to tell the browser to not count the scrollbar as being part of the layout, like Mac OS does.

@PVince81
Copy link
Contributor Author

It seems there used to be a value called overflow: overlay that would achieve the desired result, but it has been deprecated: https://developer.mozilla.org/en-US/docs/Web/CSS/overflow

@PVince81
Copy link
Contributor Author

Seems we have an even bigger undiscovered problem: the share indicators column doesn't have a fixed width for neither the header nor the body... It seems we'd need the same calculation here as well.

@kulmann
Copy link
Member

kulmann commented Feb 13, 2020

Seems we have an even bigger undiscovered problem: the share indicators column doesn't have a fixed width for neither the header nor the body... It seems we'd need the same calculation here as well.

@PVince81 what about calculating the width of #files-list-container and setting it as max-width on #files-table-header? So that you don't operate on column level, but instead just for the whole divs?

@PVince81
Copy link
Contributor Author

I have an ugly but working solution now: copy all column widths from the table rows to the header to have consistent sizes.

@kulmann processing column-wise is necessary because of the share indicators column width which is not predictable when using extensions (it's not always just two icons). Adjusting the parent's width wouldn't be enough to help with header label alignments.

I'm unsure yet whether I'll want to merge this solution but please do have a look and let me know what you think. @LukasHirt @kulmann

@PVince81
Copy link
Contributor Author

if this become a thing for more tables (at least the ones with RecycleScroller), we could consider wrapping all this ugliness into a separate vue component in the future...

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 13, 2020

After some thought I think I'll still try the other solution of setting the width of the parent, the share indicator column is likely not relevant there.

  • try approach about setting parent width

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

See comments. Edit: lol, outdated. ^^

apps/files/src/components/FileList.vue Outdated Show resolved Hide resolved
apps/files/src/components/FileList.vue Outdated Show resolved Hide resolved
apps/files/src/components/FileList.vue Outdated Show resolved Hide resolved
@PVince81
Copy link
Contributor Author

@kulmann you were right, it seems to work fine! See last commit.

If all agree I'll squash.

There's a little UIKit bug I found while switching between multiple responsive mode: in some cases it seems UIKit is setting "uk-first-column" on the wrong column, here "Updated" which causes an additional top padding. I'd consider this unrelated but we could probably work it around later.

@kulmann
Copy link
Member

kulmann commented Feb 13, 2020

@kulmann you were right, it seems to work fine! See last commit.

If all agree I'll squash.

There's a little UIKit bug I found while switching between multiple responsive mode: in some cases it seems UIKit is setting "uk-first-column" on the wrong column, here "Updated" which causes an additional top padding. I'd consider this unrelated but we could probably work it around later.

Yes, please squash. On mac this works fine. When you squashed I'll review again tomorrow, early in the morning.

@PVince81 PVince81 force-pushed the fix-responsive-filename-width branch from 8641381 to 95400cf Compare February 14, 2020 07:38
@PVince81
Copy link
Contributor Author

As discussed:

  • squashed
  • removed "flip: false" from the changeset as it caused weird menu positioning

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Awesome!

@PVince81
Copy link
Contributor Author

I'll add a changelog entry for this as a bugfix since it did not make it into 0.4.0

- Removed uk-width-small and now using the icon width for column
alignment. This gives more space in responsive mode for the file name
column.
- Removed the "More" column header label which felt redundant.
- Adjusted file actions button id to not mention size any more, now that
we only have a single size.
- Share indicator column now always present even when indicators are not
  rendered, for consistency with the header.
- On resize, copy the table body row width to the header row to make sure
that headers stay aligned when a scrollbar appears in the body as
the scrollbar does not cover the header part as the latter is
fixed/frozen on scroll.
@PVince81 PVince81 force-pushed the fix-responsive-filename-width branch from 95400cf to 7aa2369 Compare February 14, 2020 13:13
@PVince81 PVince81 merged commit c630be1 into master Feb 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-responsive-filename-width branch February 14, 2020 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Responsive files view has file names truncated
2 participants