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

Changed all the html into go using go-elem #2161

Merged

Conversation

amha-mersha
Copy link
Contributor

Created templates package in ./hscontrol/templates. Moved the registerWebAPITemplate into the templates package as a function to be called.

Replaced the apple and windows html files with go-elem.

  • have read the CONTRIBUTING.md file
  • raised a GitHub issue or discussed it on the projects chat beforehand
  • added unit tests
  • added integration tests
  • updated documentation if needed
  • updated CHANGELOG.md

@kradalby
Copy link
Collaborator

@nblock if you have some time, can you do a quick skim of the content for windows and apple?

@kradalby
Copy link
Collaborator

@amha-mersha can you delete the HTML templates so we are sure noone will update them in error?

@amha-mersha amha-mersha force-pushed the amha.refactoring_html_codes_using_goelem branch from ca8521a to 1e8fc3e Compare September 30, 2024 16:51
@nblock
Copy link
Collaborator

nblock commented Sep 30, 2024

@nblock if you have some time, can you do a quick skim of the content for windows and apple?

Sure, but I got this on nix build:

error: hash mismatch in fixed-output derivation '/nix/store/xj7li14vnd7v6rnmlj5k7v2rqgm7hxw7-headscale-1e8fc3e-go-modules.drv':
         specified: sha256-+8dOxPG/Q+wuHgRwwWqdphHOuop0W9dVyClyQuh7aRc=
            got:    sha256-U5u4tRcCQ+aRIFzOOm0KWRX4BitWKwiHtq5m2DadNtA=
error: 1 dependencies of derivation '/nix/store/f2ds1pcql2haf3s2z3m68mfikcga7lix-headscale-1e8fc3e.drv' failed to build

I manually fixed the checksum in flake.nix, but this is probably not the correct fix?

Copy link
Collaborator

@nblock nblock left a comment

Choose a reason for hiding this comment

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

Some spaces are missing between element, probably due to minified output (left a few screenshots where spotted).

elem.Body(attrs.Props{
attrs.Style : bodyStyle.ToInline(),
},
headerOne("headscale: Windows configuration"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

space missing:

image

headerTwo("GUI"),
elem.Ol(nil,
elem.Li(nil,
elem.Text("Install the official Tailscale iOS client from the"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The space between from the and the appstore link is missing:

image

),
elem.Li(nil,
elem.Text("Open Tailscale and make sure you are"),
elem.I(nil, elem.Text("not")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

When rendered, there is no space between those elements:

image

),
elem.Li(nil,
elem.Text("Restart the app by closing it from the iOS app switcher, open the app and select the regular sign in option"),
elem.I(nil, elem.Text("(non-SSO)")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing space:

image

),
),
elem.Li(nil,
elem.Text(fmt.Sprintf(`Enter "%s" under "Alternate Coordination Server URL`,url)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The closing " is missing after Coordination Server URL

@amha-mersha amha-mersha force-pushed the amha.refactoring_html_codes_using_goelem branch from 1e8fc3e to 94043af Compare September 30, 2024 18:19
@amha-mersha
Copy link
Contributor Author

@nblock I think it is now fixed

@nblock
Copy link
Collaborator

nblock commented Oct 1, 2024

@nblock I think it is now fixed

That was quick, thx!

Found one additional location where the space is lost:

image

@amha-mersha amha-mersha force-pushed the amha.refactoring_html_codes_using_goelem branch from 94043af to 5201cdf Compare October 2, 2024 13:51
@amha-mersha
Copy link
Contributor Author

how about now @nblock . sorry to take your time for this things

@kradalby
Copy link
Collaborator

kradalby commented Oct 2, 2024

Linter and TestAuthKeyLogoutAndRelogin is expected, but can you have a look at updating the build flake sha?

@kradalby
Copy link
Collaborator

kradalby commented Oct 3, 2024

A rebase should also fix the failing integration tests.

@amha-mersha
Copy link
Contributor Author

So should I first rebase and commit

@kradalby
Copy link
Collaborator

kradalby commented Oct 3, 2024

yep that sounds best

            Created templates package in ./hscontrol/templates.
            Moved the registerWebAPITemplate into the templates package as a function to be called.

            Replaced the apple and windows html files with go-elem.
@amha-mersha amha-mersha force-pushed the amha.refactoring_html_codes_using_goelem branch from 5201cdf to 5c226e1 Compare October 3, 2024 11:13
@amha-mersha
Copy link
Contributor Author

I have rebased and pushed again

@kradalby
Copy link
Collaborator

kradalby commented Oct 3, 2024

Think the nix flake needs to be update again, haha, we are very close!

@amha-mersha
Copy link
Contributor Author

do I update that and how

@kradalby
Copy link
Collaborator

kradalby commented Oct 3, 2024 via email

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
@kradalby kradalby enabled auto-merge (squash) October 4, 2024 10:39
@kradalby
Copy link
Collaborator

kradalby commented Oct 4, 2024

I fixed it up and will merge it when the test runs :) Thanks @amha-mersha, if you feel like more challenges there is one linked in the issue for OIDC page that is trickier because it has more CSS.

@amha-mersha
Copy link
Contributor Author

Thank you @kradalby for taking the time and guiding me. And for the other one, sure I look into it and get back to you

@kradalby kradalby merged commit 24e7851 into juanfont:main Oct 4, 2024
118 of 121 checks passed
@amha-mersha
Copy link
Contributor Author

Could you share me the issue, I couldn't find it.

@kradalby
Copy link
Collaborator

kradalby commented Oct 4, 2024

#2115 @amha-mersha

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.

3 participants