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

feat(components): added support for all anchor attrs #460

Merged
merged 3 commits into from
Jan 27, 2025

Conversation

alexpricedev
Copy link
Contributor

⚠️ Sorry for all of the formatting changes. I have prettier installed and I checked it was picking up the .prettierrc.yaml but on save it still seemed to want to update a load of formatting....
Main changes are:
- LOC:118-131 in packages/nuekit/src/layout/components.js
- LOC: 68-98 in CONTRIBUTING.md

Summary

This PR adds support for passing in all allowed attrs to an anchor tag (<a>). I also added some extra notes to the CONTRIBUTING.md Local Development section as it took me a few minutes to figure out the bun linking and I thought the extra notes would help other first time contributors.

The a attrs are useful for adding properties like target="_blank" to navigation items, eg:

navigation:
  footer:
    - © Nue: https://nuejs.org/
      target: _blank
      rel: noopener noreferrer
Rendered output
image

Notes

  • Test case added
  • Tests suite ran and completed with 100% pass rate

@alexpricedev

This comment was marked as resolved.

@alexpricedev

This comment was marked as resolved.

@nobkd

This comment was marked as resolved.

@alexpricedev alexpricedev force-pushed the feat/link-components-attrs branch from e2b6d74 to e1c9e9f Compare January 26, 2025 20:19
@nobkd nobkd requested a review from tipiirai January 27, 2025 01:35
const body = !thumb
? time + elem('a', { href: url }, h2 + p)
: // figure
elem(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should stick to compact formatting in elem() calls consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll have a look at the prettier config and see if I can enforce that 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent some time looking at this and really there isn't a good way of doing it without building a prettier plugin. I've never done that and would be up for trying it if you're interested?

@tipiirai
Copy link
Contributor

The changes made here seem legit. In the future, I prefer more focused pull requests. For example, this PR should have been split in two: 1) updates to contributing 2) anchor attrs. More convenient.

But in any case: thank you for the contribution! Will merge now

@tipiirai tipiirai merged commit e7cf831 into nuejs:master Jan 27, 2025
8 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.

3 participants