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

Html edits from bootstrap 3 to bootstrap 5 #1058

Merged
merged 43 commits into from
Jun 6, 2024

Conversation

brownlenox
Copy link
Contributor

@brownlenox brownlenox commented May 9, 2024

Removed the classes hiding the menu and adjusted it for a horizontal layout.
Other Fixes:

Removed extra space above the navbar.
Adjusted the width of the assets table.
Hid the URL column in the sensor table again (clickable rows preserved).


  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

@Flix6x
Copy link
Contributor

Flix6x commented May 10, 2024

Thanks for contributing. Our test pipeline shows the following error:

jinja2.exceptions.TemplateSyntaxError: Encountered unknown tag 'comment'.

Please move the comments (that summarize the changes) into the PR description.

Are you working with a running FlexMeasures installation (e.g. with data from the toy tutorial), or with html pages only? I tested the new UI locally, but many elements end up misplaced. Some of the regressions (viewed on my laptop, 1920x1080, on pages that are not throwing the above Jinja exception):

  • Large white margin at the top of the page.
  • Contents of (hidden) modal divs (such as in our footer, and also on the /tasks page) are now always visible on the page.
  • Expandable menu item listing (under the Tasks menu item) are always expanded instead of collapsed until clicked on.
  • Menu appears vertically stacked instead of horizontally.
  • Less responsive (e.g. menu no longer collapses into hamburger icon on small screen size).
  • Floating table headers, when scrolling down, become floating on the middle of the page rather than at the top (seems to be related to the large white margin at the top).
  • Footer contents are no longer center aligned.
  • The replay button on the /sensor/<id> page incorrectly resizes when clicked on.

@Flix6x Flix6x added the UI label May 10, 2024
@brownlenox
Copy link
Contributor Author

brownlenox commented May 10, 2024 via email

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

The comments are helpful, but I think they should live in the PR description, to be documented there, and not in the code.

Can you move them?

flexmeasures/ui/templates/base.html Outdated Show resolved Hide resolved
@brownlenox
Copy link
Contributor Author

brownlenox commented May 10, 2024 via email

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Thanks, now we are loading the modern bootstrap!

I'm not sure which comments by @Flix6x from above still apply now. Here some things I see:

  • Menu not displaying (element is set to be collapsed with class "collapse navbar-collapse navbar-right" and ID #bs-example-navbar-collapse-1), but even when i display it, it's vertical.
  • White space above menu bar
  • Assets page: table seems too narrow
  • Sensor table on asset page shows URL column, which we were hiding on purpose (we use it to make each row a clickable link)

I attach screenshots for each:

Screenshot from 2024-05-11 01-15-13

Screenshot from 2024-05-11 01-15-41

Screenshot from 2024-05-11 01-16-46

@brownlenox
Copy link
Contributor Author

brownlenox commented May 11, 2024 via email

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

  • The menu is now horizontal and shows, but it is not styled. The dropdown for tasks isn't working.
  • The breadcrump navigation on the asset page should have some padding on the left, it is only clickable at the bottom edge, and the dropdown at the right side doesn't work.
  • The Create new asset" button on the asset overview page is vertically moved to the middle, should be at the top.
  • The accounts page still shows the "URL" column which should be hidden.
  • The /logged-in-user page layout is broken

Screenshot from 2024-05-14 09-47-55

Screenshot from 2024-05-14 09-48-59

Screenshot from 2024-05-14 10-13-16

Please compare the current layout to the previous one.

flexmeasures/ui/static/js/flexmeasures.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@brownlenox brownlenox left a comment

Choose a reason for hiding this comment

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

These are changes made regarding the requested changes.

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Thanks.

  • The menu seems to have some padding or margin (top and bottom) which make the highlighting of the currently selected item not look good. Also, action buttons (e.g. create new asset) and the asset breadcrumb are then too close to the menu. Check #topnavbar, I found for instance -bs-navbar-padding-y`.
  • The sensor table on the asset page still shows the hidden URL column. Same for users and assets tables on accounts page.
  • Breadcrumbs are now wrapping and the dropdown is gone.
  • On logged-in-user page, the audit log buttons are too low.

Some of these issues are needed to be checked on all pages, not just foxed on one, apparently.

Uploading Screenshot from 2024-05-14 12-24-17.png…

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

I believe we are getting closer.

  • The menu is still too wide vertically. I can fix that in my console removing --bs-navbar-padding-y: 0.5rem;, as I said earlier.
  • The user page seems broken now.
  • The bread crumps on the asset page seem to work better, but they still wrap vertically.

Screenshot from 2024-05-14 23-52-47
Screenshot from 2024-05-14 23-52-13
Screenshot from 2024-05-14 23-51-27

Copy link
Contributor Author

@brownlenox brownlenox left a comment

Choose a reason for hiding this comment

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

Made those changes, but i can't seem to find, this --bs-navbar-padding-y: 0.5rem, to remove it from my end to ensure the menu is not too large vertically

@nhoening
Copy link
Contributor

It's in the bootstrap code actually. I just found it with the dev toolbar.
I'm not sure what the best way is to set menu height better, but maybe overwriting this with !important in our own CSS would work?

Screenshot from 2024-05-15 11-33-00

@brownlenox
Copy link
Contributor Author

It's in the bootstrap code actually. I just found it with the dev toolbar. I'm not sure what the best way is to set menu height better, but maybe overwriting this with !important in our own CSS would work?

Screenshot from 2024-05-15 11-33-00

I have enforced it by padding-top and bottom of 0

@nhoening
Copy link
Contributor

This is the buttons under the table right now:

Screenshot from 2024-05-29 16-51-30

Copy link
Contributor Author

@brownlenox brownlenox left a comment

Choose a reason for hiding this comment

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

thanks

Copy link
Contributor Author

@brownlenox brownlenox left a comment

Choose a reason for hiding this comment

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

removed unnecessary css

flexmeasures/ui/static/css/flexmeasures.css Outdated Show resolved Hide resolved
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Great, everything seems to work fine. Thanks!

I approve and have just one question left, which you might know an answer to.

flexmeasures/ui/templates/base.html Show resolved Hide resolved
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nhoening nhoening merged commit c21e5a0 into FlexMeasures:main Jun 6, 2024
2 checks passed
victorgarcia98 added a commit that referenced this pull request Jun 7, 2024
This reverts commit c21e5a0.

Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
victorgarcia98 pushed a commit that referenced this pull request Jun 21, 2024
* Html edits from bootstrap 3 to bootstrap 5

* Correction on the cdn links and script link

* comments moved to PR description

* fixed Assets page: table seems too narrow and others..

* Menu enhancement etc...

* Compared current layout and made changes requested changes

* Compared current layout and made changes requested changes

* Compared current layout and made changes requested changes

* Compared current layout and made changes requested changes

* ensuring similarity between the versions

* Correction of requested changes

* Adjusting menu height

* Adjusting menu height

* Resolving the latest issues

* Correction of table, menu hamburger etc

* Fixed everything

* Fixes

* updated fixes

* other updated fixes

* flex and s margin

* breadcrumb toggle

* page load on clicked breadcrumb dropdown

* style attributes to custom css file

* auto-commit

* changes on requested changes

* changes on requested changes

* auto-commit

* auto-commit

* custom css to flexmeasures.css

* custom css to flexmeasures.css

* custom css to flexmeasures.css

* breadcrumb

* auto-commit

* Fix wrong click event on breadcrumb dropdown

Signed-off-by: Nicolas Höning <nicolas@seita.nl>

* add back space between 'active' and  'users'

Signed-off-by: Nicolas Höning <nicolas@seita.nl>

* select date-div, margin

* removed unnecessary style

* improve/fix some indentation

Signed-off-by: Nicolas Höning <nicolas@seita.nl>

---------

Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Co-authored-by: Nicolas Höning <nicolas@seita.nl>
Flix6x added a commit that referenced this pull request Aug 10, 2024
Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x mentioned this pull request Aug 10, 2024
Flix6x added a commit that referenced this pull request Aug 11, 2024
* style: remove non-breaking spaces from base template

Signed-off-by: F.N. Claessen <felix@seita.nl>

* fix: update display property for Bootstrap >= 4

Signed-off-by: F.N. Claessen <felix@seita.nl>

* docs: put back inline comment removed in #1058

Signed-off-by: F.N. Claessen <felix@seita.nl>

* docs: expand on rationale in inline comment

Signed-off-by: F.N. Claessen <felix@seita.nl>

* docs: changelog entry

Signed-off-by: F.N. Claessen <felix@seita.nl>

* fix: breakpoint for collapsing menu and moving tooltip to caption should coincide also for intermediate screen sizes

Signed-off-by: F.N. Claessen <felix@seita.nl>

---------

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x mentioned this pull request Aug 23, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants