Skip to content

Conversation

BenjaminCharmes
Copy link
Contributor

@BenjaminCharmes BenjaminCharmes commented Dec 3, 2024

Closes #32:

  • New <button /> class used to close Modal
  • Replace ml-2 (doesn't exist in Bootstrap v5) by gap-2 (d-flex) for DynamicDataTableButtons (could also use me-2 from bootstrap instead) (Bootstrap left and right replaced by start and end)
  • I had color: black to FormattedItemName because badge light display text in white now
  • Edit every Modal (Item, Batch, Collection, Equipment ...) to stay the same
  • Edit every datablock (and also Bokeh block with querySelector)
  • and more ...

Closes #620:

  • CompactConstituentTable.vue is now responsive.
  • The EditPage and CollectionPage navbar now use a dropdown on small screen
  • DynamicDataTabeButtons also use a dropdown on small screen
  • and more ...

Cypress:

  • Improving tests using Modal and vue-select
  • Add a few utility functions
  • Lot of refactoring
  • There are still a lot of cy.wait() that could (should?) be removed (I haven't removed them, waiting feedback).

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.09%. Comparing base (e8b7c5a) to head (4ef783e).
Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1028   +/-   ##
=======================================
  Coverage   71.09%   71.09%           
=======================================
  Files          65       65           
  Lines        4300     4300           
=======================================
  Hits         3057     3057           
  Misses       1243     1243           
Files with missing lines Coverage Δ
pydatalab/src/pydatalab/bokeh_plots.py 81.81% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

cypress bot commented Dec 3, 2024

datalab    Run #3362

Run Properties:  status check passed Passed #3362  •  git commit 92e8a06b7f ℹ️: Merge 4ef783ec6825c2a9d9385a4ffc7d4a482d013a3f into e8b7c5a7d9a67867ff8d52781906...
Project datalab
Branch Review bc/update-bootstrap5
Run status status check passed Passed #3362
Run duration 08m 45s
Commit git commit 92e8a06b7f ℹ️: Merge 4ef783ec6825c2a9d9385a4ffc7d4a482d013a3f into e8b7c5a7d9a67867ff8d52781906...
Committer Ben Charmes
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 504
View all changes introduced in this branch ↗︎

@BenjaminCharmes BenjaminCharmes changed the title Update to Bootstrap v5 Ugrade to Bootstrap v5 Dec 4, 2024
@BenjaminCharmes BenjaminCharmes changed the title Ugrade to Bootstrap v5 Upgrade to Bootstrap v5 Dec 4, 2024
@BenjaminCharmes BenjaminCharmes added webapp For issues/PRs pertaining to the web interface dependency_updates For issues/PRs that update the dependencies of the package labels Dec 10, 2024
@ml-evs ml-evs force-pushed the bc/update-bootstrap5 branch from 7a936d1 to 3c10d26 Compare January 27, 2025 22:06
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

I've built and played around with the new version and only have a few minor comments of mine (and we can go through yours) -- just a comment below about the deps!

@ml-evs ml-evs self-assigned this Feb 7, 2025
@ml-evs ml-evs removed their assignment May 6, 2025
@BenjaminCharmes BenjaminCharmes force-pushed the bc/update-bootstrap5 branch 2 times, most recently from d18c840 to 38a406c Compare May 15, 2025 08:53
@BenjaminCharmes
Copy link
Contributor Author

Everything works! 🎉 I still need to refactor the e2e tests a bit (I added a lot of functions) and clean things up, and then the PR will be ready.

@BenjaminCharmes BenjaminCharmes marked this pull request as ready for review May 16, 2025 11:59
@BenjaminCharmes BenjaminCharmes requested a review from ml-evs May 16, 2025 11:59
@ml-evs
Copy link
Member

ml-evs commented May 16, 2025

I'm going to collect general comments as I explore the UI without trying to be mega picky, and without referencing any particular changes. These may also be issues present in the existing UI. I'll tag them as 🔴 (needs fixing), 🟡 (needs discussion) and 🟢 (probably fine or an existing issue, just noting the change).

  • 🟢 button-left on the DynamicDataTable seems to fill to a different height to button-right (38px vs 42px for me). There is now uniformity between e.g., the search bar and the multi-select, which wasn't the case before (i.e., a good thing!)
  • 🟡 / 🟢 The button-left and button-right groups still do not scale very nicely in small windows
  • 🟢 item badges have slightly more padding now (not necessarily bad but might need to be reduced slightly as it decreases the font size)
  • 🔴 submit button in modal has new (worse, IMO) colour -- black on rich blue rather than white on a darkish teal
  • 🔴 Router links (e.g., About | Samples | Collections) are now underlined even when they are not selected
  • 🔴 / 🟡 Bokeh plots in some blocks like XRD are now much wider by default (maybe too large), though previously I think this was hardcoded on the Python side which is also not optimal
  • 🟡 The margin for the block names in the add a block menu is quite a lot larger
  • 🔴 Synthesis information tables now reactive (great!) but the unit column is now too large and it gets broken by samples with long names as it no longer wraps (see screenshot) -- this actually breaks all the rows above it too.
    2025-05-16-155345_1367x250_scrot

Router links underlined only when selected

Router links underlined only when selected
max-length for FormattedItemName in SynthesisInformation
@BenjaminCharmes
Copy link
Contributor Author

BenjaminCharmes commented May 20, 2025

I've made most of the changes asked, whenever you have time to take a look @ml-evs .
And awaiting discussion on the others.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Great work @BenjaminCharmes, this is looking really nice from my side! I'm going to approve this now, but we should hold off merging it until the 0.5.3 release is out. Then this can be the first PR merged for the 0.6.0 series.

@ml-evs ml-evs added this to the v0.6.x milestone May 20, 2025
@ml-evs ml-evs added the on-hold label May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency_updates For issues/PRs that update the dependencies of the package on-hold webapp For issues/PRs pertaining to the web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better mobile support Update to Bootstrap 5
2 participants