feature/Issue-97: Implement the Edit on GitHub component #111

DevLab2425 · 2024-10-14T02:28:14Z

Related Issue

Fix #97

Summary of Changes

Create <app-edit-on-github> component
Create test suite for potential inputs
Create styles to make button sticky and responsive
Create initial story example

Screenshots

<= 1199px

- ** >=1200px**

Screen.Recording.2024-10-14.at.13.25.38.mov

Screen.Recording.2024-10-14.at.13.26.18.mov

Todo

Additional story examples

netlify · 2024-10-14T02:29:10Z

✅ Deploy Preview for super-tapioca-5987ce ready!

Name	Link
🔨 Latest commit	`ebb27b1`
🔍 Latest deploy log	https://app.netlify.com/sites/super-tapioca-5987ce/deploys/67113f5655a20b000867aa60
😎 Deploy Preview	https://deploy-preview-111--super-tapioca-5987ce.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/components/edit-on-github/edit-on-github.js

thescientist13

Nice, this is looking pretty good! Left some thoughts and feedback, let me know what you think. 🙏

src/components/edit-on-github/edit-on-github.js

thescientist13 · 2024-10-14T15:51:01Z

src/components/edit-on-github/edit-on-github.js

+function convertRouteToSubLink(route) {
+  if (route === "/") return "index.md"; // root of diretory === index
+
+  const DIRS = ["guides", "ecosystem", "getting-started", "hosting", "tutorials"];


So I think its fine to just have a straight conversion here instead of hardcoding everything, since this will also extend to the Docs pages as well, and don't want to have to keep updating this every time we add it on a new page and all its sub routes.

I think it should be patched into this repo but there is also an id property on each page which is a normalized output of the file name, that in hindsight might be a more useful way of converting from a route to a page path (this will get a little more organized in our new docs content coming soon to a greenwood website near you!). See the build output here as an example.

So basically, given this pages structure:

src/ pages/ index.html guides/ index.md something/ else.md

The IDs would come out as such

src/pages/index.html -> index

src/pages/guides/index.md -> guides-index

src/pages/guides/something/else.md -> guides-something-else

So I think with that as more of a predictable pattern, you can always test if something is the root by the presence of index at the end, which I think should make the conversions a bit easier.

I gave this a shot, but it seems that the ID is not available, but maybe I'm using it incorrectly.

<app-edit-on-github route="${globalThis.page.id}"></app-edit-on-github>

Results in the attribute value being "${globalThis.page.id}", however, the route does return the property value correctly.

hmm, ok let me take a look. might be a missing patch set, and I can take a look at that Wednesday evening after out meeting. (I'll plan to do a fresh RC release in the next couple of days as part of #107 )

Or if you want to try with route now, but it just might be a little trickier to get the conversion right. Will leave it up to you.

src/components/edit-on-github/edit-on-github.module.css

thescientist13 · 2024-10-14T16:00:05Z

src/layouts/guides.html

@@ -71,6 +76,17 @@
        }
      }

+      .content-footer {


maybe we can target specifically with the tag name instead of a new class? I've been doing that with some of the other CEs on this page, so might make the most sense for consistency.

Also, same feedback here re: using Open Props where possible.

Happy to make the change, but so to be clear, this class is applied to a wrapper around the <app-edit-on-github> element. This is to provide a block-level container to the anchor tag and allow for the layout styles.

<body> ... <div class="content-footer"> <app-edit-on-github /> </div> </body>

Without the applied class, I'd apply styles like the following, which may impact future enhancements to the guides.html layout if we were to add additional <div> under .content-outlet.

.content-outlet > div { .... }

Right, but I'm guessing why do we need the wrapper at all, if we just want to target the custom element anyway? I agree that making it sticky and doing all the positioning from the parent makes sense, just not sure why we need to wrap at all? (For example, now I see now class at all on it, just an empty <div>?)

Is it maybe because CEs are display:inline by default, and the <div> gives you a display:block?

Ah, gotcha. I used the wrapper as a means of positioning the anchor and giving it the margin. Let me remove it and give it a shot.

src/layouts/guides.html

src/components/edit-on-github/edit-on-github.spec.js

thescientist13 · 2024-10-15T19:55:55Z

src/components/edit-on-github/edit-on-github.stories.js

+import "./edit-on-github.js";
+import pages from "../../stories/mocks/graph.json";
+
+export default {


similar comment here re: needing to actually mock out content data like this?

thescientist13 · 2024-10-15T19:57:27Z

src/components/edit-on-github/edit-on-github.js

+function convertRouteToSubLink(route) {
+  if (route === "/") return "index.md"; // root of diretory === index
+
+  const DIRS = ["guides", "ecosystem", "getting-started", "hosting", "tutorials"];


hmm, ok let me take a look. might be a missing patch set, and I can take a look at that Wednesday evening after out meeting. (I'll plan to do a fresh RC release in the next couple of days as part of #107 )

Or if you want to try with route now, but it just might be a little trickier to get the conversion right. Will leave it up to you.

thescientist13 · 2024-10-15T20:02:40Z

src/layouts/guides.html

@@ -71,6 +76,17 @@
        }
      }

+      .content-footer {


Right, but I'm guessing why do we need the wrapper at all, if we just want to target the custom element anyway? I agree that making it sticky and doing all the positioning from the parent makes sense, just not sure why we need to wrap at all? (For example, now I see now class at all on it, just an empty <div>?)

Is it maybe because CEs are display:inline by default, and the <div> gives you a display:block?

thescientist13 · 2024-10-15T20:04:05Z

src/layouts/guides.html

+        margin: var(--size-px-6) 0 var(--size-px-2) 0;
+      }
+
+      .content-outlet div[class*="edit-on-github"] a {


hmm, looks like we're piercing into the CE and styling the <a> from the outside? Any reason this style can't be co-located within the module.css file as well?

Cleaned this up, and moved to the module file.

thescientist13

Sweet! Just made a small refactor to the route -> edit link conversion since we can rely on having a predictable IA (Information Architecture) and so with that constraint in place, this should now easily scale out to the Docs page as well, which should be pretty good for us for a while. All the tests and storybook still passed. 💯

Let me know your thoughts, otherwise will merge away soon. 👌

DevLab2425 changed the title ~~Issue 97 edit on GitHub link~~ Issue-97: Implement the Edit on GitHub component Oct 14, 2024

DevLab2425 force-pushed the issue-97-edit-on-github-link branch 2 times, most recently from 4a797d9 to a6fa1f0 Compare October 14, 2024 05:12

DevLab2425 marked this pull request as ready for review October 14, 2024 05:19

DevLab2425 requested a review from thescientist13 October 14, 2024 05:19

DevLab2425 commented Oct 14, 2024

View reviewed changes

src/components/edit-on-github/edit-on-github.js Outdated Show resolved Hide resolved

DevLab2425 force-pushed the issue-97-edit-on-github-link branch from 1f7083a to 933fd6a Compare October 14, 2024 06:13

thescientist13 added the enhancement New feature or request label Oct 14, 2024

thescientist13 reviewed Oct 14, 2024

View reviewed changes

DevLab2425 added the hacktoberfest-accepted Used to participate in Hacktoberfest label Oct 14, 2024

thescientist13 self-assigned this Oct 15, 2024

thescientist13 reviewed Oct 15, 2024

View reviewed changes

thescientist13 changed the title ~~Issue-97: Implement the Edit on GitHub component~~ feature/Issue-97: Implement the Edit on GitHub component Oct 17, 2024

thescientist13 approved these changes Oct 17, 2024

View reviewed changes

DevLab2425 added 15 commits October 17, 2024 12:46

Issue-97: Create component and initial tests

06e4f38

Issue-97: Create styles, story for component

9517312

Issue-97: Initial implementation into page

9ec7247

Issue-97: Sticky footer

608293b

Issue-97: Extend styles to be responsive

b256e57

Issue-97: Revert work on footer

0b47c1b

Issue-97: Clean up

aee7bbb

Issue-97: Correct prettier errors

68618fe

Issue-97: Include additional stories for variants

1dea7f0

Issue-97: Target edit view

f9bb454

Issue-97: Remove unnecessary label prop

5036a35

Issue-97: Restyle link and wrapper

187b009

Issue-97: Clean up story template

f250d47

Issue-97: Remove unnecessary comment

2bf543b

Issue-97: Resolve failing tests

1b672db

DevLab2425 and others added 3 commits October 17, 2024 12:46

Issue-97: Refactor guides implmentation

cc23014

Issue-97: Update story to remove label example

ccfcbbf

general refactoring to align with site information architecture

ebb27b1

thescientist13 force-pushed the issue-97-edit-on-github-link branch from 22d7eda to ebb27b1 Compare October 17, 2024 16:46

thescientist13 merged commit 9f32e2f into main Oct 17, 2024
5 checks passed

thescientist13 deleted the issue-97-edit-on-github-link branch October 17, 2024 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/Issue-97: Implement the Edit on GitHub component #111

feature/Issue-97: Implement the Edit on GitHub component #111

DevLab2425 commented Oct 14, 2024 •

edited

Loading

netlify bot commented Oct 14, 2024 •

edited

Loading

thescientist13 left a comment

thescientist13 Oct 14, 2024 •

edited

Loading

DevLab2425 Oct 14, 2024

thescientist13 Oct 15, 2024 •

edited

Loading

thescientist13 Oct 14, 2024

DevLab2425 Oct 14, 2024 •

edited

Loading

thescientist13 Oct 15, 2024 •

edited

Loading

DevLab2425 Oct 16, 2024

thescientist13 Oct 15, 2024

thescientist13 Oct 15, 2024 •

edited

Loading

thescientist13 Oct 15, 2024 •

edited

Loading

thescientist13 Oct 15, 2024

DevLab2425 Oct 16, 2024

thescientist13 left a comment •

edited

Loading

feature/Issue-97: Implement the Edit on GitHub component #111

feature/Issue-97: Implement the Edit on GitHub component #111

Conversation

DevLab2425 commented Oct 14, 2024 • edited Loading

Related Issue

Summary of Changes

Screenshots

Todo

netlify bot commented Oct 14, 2024 • edited Loading

✅ Deploy Preview for super-tapioca-5987ce ready!

thescientist13 left a comment

Choose a reason for hiding this comment

thescientist13 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

DevLab2425 Oct 14, 2024

Choose a reason for hiding this comment

thescientist13 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

thescientist13 Oct 14, 2024

Choose a reason for hiding this comment

DevLab2425 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

thescientist13 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

DevLab2425 Oct 16, 2024

Choose a reason for hiding this comment

thescientist13 Oct 15, 2024

Choose a reason for hiding this comment

thescientist13 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

thescientist13 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

thescientist13 Oct 15, 2024

Choose a reason for hiding this comment

DevLab2425 Oct 16, 2024

Choose a reason for hiding this comment

thescientist13 left a comment • edited Loading

Choose a reason for hiding this comment

DevLab2425 commented Oct 14, 2024 •

edited

Loading

netlify bot commented Oct 14, 2024 •

edited

Loading

thescientist13 Oct 14, 2024 •

edited

Loading

thescientist13 Oct 15, 2024 •

edited

Loading

DevLab2425 Oct 14, 2024 •

edited

Loading

thescientist13 Oct 15, 2024 •

edited

Loading

thescientist13 Oct 15, 2024 •

edited

Loading

thescientist13 Oct 15, 2024 •

edited

Loading

thescientist13 left a comment •

edited

Loading