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

COMPATIBILITY: Modernize the component #11

Merged
merged 16 commits into from
Jul 10, 2024

Conversation

Arkshine
Copy link
Contributor

@Arkshine Arkshine commented Jul 9, 2024

This PR modernizes the components:

  • Use the new header API
  • Convert all the widgets to glimmer components
  • Fix a few UX issues
  • Add acceptance tests

The HTML structure and CSS classes should be the same.

NVIDIA_Share_Xnvy8FsB5i.mp4

@@ -81,7 +81,7 @@
}

// hide on scroll when nav is positioned to left of header:
.title .custom-header-links.scrolling {
.d-header .custom-header-links.scrolling {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plugin outlet wrapper is not inside .title.

@angusmcleod
Copy link
Member

@Arkshine Amazing stuff!

@angusmcleod angusmcleod requested a review from nolosb July 10, 2024 07:36
@angusmcleod
Copy link
Member

@nolosb Would you mind reviewing this one?

@merefield
Copy link
Member

merefield commented Jul 10, 2024

@Arkshine checks are failing currently?

@merefield merefield changed the title DEV: Modernize the component COMPATIBILITY: Modernize the component Jul 10, 2024
@Arkshine
Copy link
Contributor Author

Arkshine commented Jul 10, 2024

@Arkshine checks are failing currently?

I tried to update the CI linting workflow. I'm unsure if it will work.
Linting requires at least node >= 18, and I added the missing gjs and hbs extensions to eslint and prettier.

I think we can use the Discourse theme reusable workflow here. It refers to https://github.com/discourse/.github/blob/main/.github/workflows/discourse-theme.yml.

Arkshine

This comment was marked as duplicate.

@@ -25,6 +25,7 @@ export default class CustomHeaderDropdown extends Component {
<li
class="custom-header-dropdown-link"
title={{@item.title}}
role="button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the "Interaction added to non-interactive element" error.
https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-invalid-interactive.md

Ideally, I think we want to use <a> link here for better accessibility.
That's something I will see to change in another PR.

@angusmcleod angusmcleod requested review from nathan-nz and removed request for nolosb July 10, 2024 13:48
@angusmcleod
Copy link
Member

@nolosb Is currently unavailable, however Github is suggesting @nathan-nz review this :)

Screenshot 2024-07-10 at 15 48 19

What do you think @nathan-nz?

@nathan-nz
Copy link
Contributor

@nolosb Is currently unavailable, however Github is suggesting @nathan-nz review this :)
What do you think @nathan-nz?

I'm no dev (as you know!) so it won't be an expert review. But I am very happy to approve it as it has had your and @merefield's eyes on it (and @Arkshine is trustworthy).

Copy link
Contributor

@nathan-nz nathan-nz 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 your excellent work @Arkshine

@nathan-nz
Copy link
Contributor

@Arkshine - I'm not sure what this means:
image
Is this something you can address?

@Arkshine
Copy link
Contributor Author

Arkshine commented Jul 10, 2024

@nathan-nz Yes, I've removed the workflow. Since we are using the Discourse reusable workflow, and the test part is already included, it was a duplicate. https://github.com/discourse/.github/blob/main/.github/workflows/discourse-theme.yml#L109

@nathan-nz nathan-nz merged commit cf2593e into paviliondev:main Jul 10, 2024
4 checks passed
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.

4 participants