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

Feature/3331 dynamic column width #6640

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ludij
Copy link
Contributor

@ludij ludij commented Jan 1, 2025

Summary

Following the descriptions in the linked ticket, I have updated the styles for the board, overview, stack and search results cards, so that they behave more responsive according to the space available. As a side-effect it also fixed issue #6457.

On top of that, this PR contains the following changes, which aren't necessarily part of the issue, but I thought they'd fit in:

  • aligned design for board and overview
  • aligned spacing inside stack between header, cards and new card form
  • refactored how border radius is achieved on card image
  • added v-for loop in overview to get rid of code duplication
  • fixed border around add list
  • used existingIndex to define existingCard to avoid extra lookup
  • fixed binding of title attibute on stack column title for boards
  • improved accessibility attributes on column for overview
  • changed title phrasing of search results depending on view
new old
1a overview new 1b overview old
2a board new 2b board old

TODO

N/A

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

@ludij ludij force-pushed the feature/3331-dynamic-column-width branch 4 times, most recently from 0a44b33 to 6a6d15e Compare January 1, 2025 12:59
@juliusknorr
Copy link
Member

juliusknorr commented Jan 3, 2025

Thanks a lot for your contribution. This looks quite good. I'd love to collect some feedback from designers like @marcoambrosini on that.

The code changes look sane to me, also some nice refactorings along the way. I'll give this a test run later. Can you maybe address the stylelint and eslint errors that CI reported?

@juliusknorr
Copy link
Member

Is the change of the scroll container intentional? I'm personally not a fan of scrolling the full board area and would rather prefer to stick with the per column scrolling we used to have before.

@ludij ludij force-pushed the feature/3331-dynamic-column-width branch from 3217cb7 to 3214277 Compare January 3, 2025 09:30
@juliusknorr
Copy link
Member

Just noticed the merge commit, could you do a rebase to the main branch instead so we have a clean git history of your changes?

@ludij
Copy link
Contributor Author

ludij commented Jan 3, 2025

Thanks for your review! I've just added a commit to fix the linting issue. Regarding the scrolling, my change is intentional, but of course I can add an new commit so that scrolling is inside the column again. I should have time during the weekend to do that. I'll do a rebase too!

@juliusknorr
Copy link
Member

Regarding the scrolling, my change is intentional, but of course I can add an new commit so that scrolling is inside the column again. I should have time during the weekend to do that. I'll do a rebase too!

That would be great, thanks a lot already. :)

@ludij ludij force-pushed the feature/3331-dynamic-column-width branch 2 times, most recently from d4b32fc to bf36ebb Compare January 5, 2025 22:19
@marcoambrosini
Copy link
Member

Hi @ludij, I just had a quick look at our competitors and it seems that varying the with of the columns is not a very common practice. These columns are usually fixed in width and horizontal scrolling is used for overflowing lists. @juliusknorr what do you think about that?
That said, in principle this seems like a sane change to me design-wise, I would just make sure to the min and max values not too far apart from each other to not completely change appearance because that could be disorienting.
I need some time to setup my development environment again to properly test it. I'll come back for a review tomorrow.
Thanks a lot for for picking this up :)

@ludij
Copy link
Contributor Author

ludij commented Jan 8, 2025

Hi @ludij, I just had a quick look at our competitors and it seems that varying the with of the columns is not a very common practice. These columns are usually fixed in width and horizontal scrolling is used for overflowing lists. @juliusknorr what do you think about that?

It's pretty easy to update this code so that the column width is fixed. Imho my refactor probably makes sense either way, because it allows the widths and spacings for the different parts to be maintained from a single source (variables.scss (at least that's what I aimed for :) )), whereas before it was needed to adjust multiple files and lines.

That said, in principle this seems like a sane change to me design-wise, I would just make sure to the min and max values not too far apart from each other to not completely change appearance because that could be disorienting. I need some time to setup my development environment again to properly test it. I'll come back for a review tomorrow.

Sounds good. I'm not so certain whether my changes to the spacing inside the card look good (with/without labels, with/without image, etc.). I also just noticed that the spacing inside the "Add new card" card doesn't align perfectly with the header and other cards in the column. I could easily fix that for the "Add new card", but it'd be great to get some feedback on the updated spacing inside the cards in general before, so I can do an update in one go.

Thanks a lot for for picking this up :)

Very happy to contribute and thanks for the feedback!

Copy link
Member

@grnd-alt grnd-alt left a comment

Choose a reason for hiding this comment

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

thanks for that contribution, I like the way the width is limited right now and would be happy getting this merged. Let's wait for a final approve from @marcoambrosini and get this in. 🎉

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply, I tested with the current parameters and everything looks good even if cards have lots of elements. Thanks a lot again for the contribution!

@grnd-alt grnd-alt self-requested a review January 16, 2025 12:46
@juliusknorr
Copy link
Member

@ludij Can you rebase your pull request to the latest main branch to resolve the conflict?

@ludij ludij force-pushed the feature/3331-dynamic-column-width branch from bf36ebb to 7451e5e Compare January 20, 2025 19:16
@ludij ludij force-pushed the feature/3331-dynamic-column-width branch from 7451e5e to c35f905 Compare January 20, 2025 19:26
@ludij ludij force-pushed the feature/3331-dynamic-column-width branch from c35f905 to 315f0cf Compare January 20, 2025 19:28
@ludij
Copy link
Contributor Author

ludij commented Jan 20, 2025

@juliusknorr + @grnd-alt Thanks for the feedback and sorry for the delay. I'm not very familiar with the rebase workflow, so it took me a few pushes, but I think everything should be fine now 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📐 At design
Development

Successfully merging this pull request may close these issues.

Dynamic column width
4 participants