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.

Sign up for GitHub

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

Jump to bottom

Link to LMS Control Panel from settings #974

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Link to LMS Control Panel from settings #974

wants to merge 1 commit into from

Conversation

SteveMicroNova
Copy link
Contributor

@SteveMicroNova SteveMicroNova commented Oct 14, 2024 •
edited
Loading

What does this change intend to accomplish?

image
Change LMS toggle switch so that it doesn't download system config on LMS deactivation, include link to LMS server when LMS Mode is active

Checklist

  • Have you tested your changes and ensured they work?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission pass linting & tests? You can test on localhost using ./scripts/test
  • If this is a UI change, have you tested it across multiple browser platforms on both desktop and mobile?

This was linked to issues Oct 14, 2024
Include a clickable link to hosted LMS Server in the same vicinity as the LMS toggle #975
Open
LMS Mode toggle downloads a config in both directions #976
Closed
klay2000
klay2000 requested changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@klay2000 klay2000 left a comment

Choose a reason for hiding this comment

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

I'm not a fan of the "follow this link"
when LMS mode is active I think a button should appear next to the switch saying "Open LMS Control Panel" This button would open the LMS control panel in a new tab.

@SteveMicroNova SteveMicroNova mentioned this pull request Oct 14, 2024
Closed
@SteveMicroNova SteveMicroNova removed a link to an issue Oct 14, 2024
LMS Mode toggle downloads a config in both directions #976
Closed
rtertiaer
rtertiaer suggested changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@rtertiaer rtertiaer left a comment

Choose a reason for hiding this comment

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

is there any way that button could look a little less haphazardly placed? I don't have any suggestions here - I know we're largely not designers - but I'd love to ensure we at least don't make things look worse. some things that I notice:

  • it's a button, but doesn't do a button-like thing - it does a link-like thing
  • there are two other types of buttons on the page; this is themed like neither of them
  • there's zero margin/padding on this button between it and the divider - feels cramped
  • it's not horizontally aligned to anything but the prior content, which feels unfinished

@SteveMicroNova
Copy link
Contributor Author

  • it's a button, but doesn't do a button-like thing - it does a link-like thing

I'm happy to make it a link again, though we'll probably have to hash that out a little further given Klayton's dislike of my previous link strategy

  • there are two other types of buttons on the page; this is themed like neither of them

Fixed

  • there's zero margin/padding on this button between it and the divider - feels cramped

This is the case for all of the buttons, but obviated by the contained styling; corrected in recent commit

  • it's not horizontally aligned to anything but the prior content, which feels unfinished

I've got no good way of fixing this as there isn't really a gridmodel in place for multi-input sections but I've taken a note from the "Upload Config" section and just made there be some spacing between the inputs

image

@SteveMicroNova SteveMicroNova mentioned this pull request Dec 5, 2024
Open
@SteveMicroNova SteveMicroNova changed the title LMSConfigPolish Link to LMS Control Panel from settings Dec 6, 2024
@SteveMicroNova SteveMicroNova force-pushed the LMSConfigPolish branch 2 times, most recently from 5b481c8 to 4af39f4 Compare December 6, 2024 16:52
@linknum23 linknum23 self-requested a review December 9, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtertiaer rtertiaer rtertiaer requested changes

@klay2000 klay2000 Awaiting requested review from klay2000

@linknum23 linknum23 Awaiting requested review from linknum23

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include a clickable link to hosted LMS Server in the same vicinity as the LMS toggle
3 participants
@SteveMicroNova @klay2000 @rtertiaer