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: The prompt to edit the current page on GitHub now triggers a contextual modal component #88

Conversation

PatrickLarocque
Copy link

@PatrickLarocque PatrickLarocque commented Sep 30, 2024

PR Author's Note

closes #80

Checklist

  • Philosophical or literary contribution (docs). Leave unchecked for
    code contribution.

    • IMPLIED CONSENT By opening this pull request and contributing
      philosophical or literary content, I accept that my writing is submitted
      under the
      ATTRIBUTION-NONCOMMERCIAL-SHAREALIKE 4.0 INTERNATIONAL,
      which:

      • Prohibits commercial reuse of the content.
      • Allows sharing, remixing, and building upon the material as long as
        attribution is given.

      I understand that my writing may be modified, remixed, and built upon by
      others within the systemphil/sphil or sPhil project, in accordance
      with the license terms, indefinitely. See
      legal code.

    • REQUIRED I have followed the
      formatting guidelines
      and verified there are no formatting bugs.
      Try markdown preview here.

    • REQUIRED I have followed the
      Chicago author-date style.

    • REQUIRED I have added or verified metadata title, description, and
      contributors at the very top of the file followed by a ## title
      heading. Additionally, I have ensured isArticle is set to true.
      Example:

      ---
      title: The Immediate Difference Between Pure Being and Pure Nothing
      description:
          Learn about the difference between being and nothing in Hegel's
          Science of Logic.
      isArticle: true
      authors: Jerry Maguire (2024)
      editors: Steve Stevenson (2023), Karen Hansen (2022)
      contributors:
      ---
      
      ## My Article Title
      Further information

      I have signed the document with my name/username under either as
      Authors, Editors or Contributors.

      Use Authors if you have created and substantially added content.
      Use Editor if you have made substantial edits or review.
      Use Contributor if you have made minor edits, reviews or
      contributions.
      If you've done multiple, pick the most weighted: Author > Editor >
      Contributor.
      If you prefer to remain anonymous, that's fine too, but note that a
      record of your contributions based on your GitHub username will exist
      here in the codebase.

    • REQUIRED I have ensured that the
      project's central bibliography
      contains the necessary bibliographical details for the citations I have
      used.

    • Optional My article is a stub or I want to actively encourage
      contribution, I've added the Stub component to the bottom of my content
      or where relevant:

      import Stub from "@/components/Stub";
      
      <Stub />;
  • If Docs contribution is unchecked: Code contribution
    (Apache version 2 license)

    All code apart of what is inside src/pages/** (excluding
    /contributing/**, _app.mdx, _document.tsx, _meta.json,
    acknowledgements.mdx, index.mdx, privacy.mdx, team.mdx, terms.mdx)
    is subject to Apache version 2 license. Basically, anything outside of
    content, literature, philosophy.

@PatrickLarocque
Copy link
Author

PatrickLarocque commented Sep 30, 2024

I used shadcn/ui for the component I made. At the moment, it doesn't play very nicely with the currenting themeing solution along w/ MUI and the other component libraries. However, given this is a next project, I feel like Shad/cn might is worth considering, given that it is maintained and supported by vercel. Although some refactoring would be needed to port existing components and theming. This is of course an open discussion and I could certainly attempt to rewrite this in MUI or HeadlessUI, please let me know.

I've provided a link to the shad/cn docs in case it is something you have not considered:
https://ui.shadcn.com/

image
image

Please note that I also included a bump to Next 14, which is likely a hasty addition and can be removed if desired.

@Firgrep
Copy link
Member

Firgrep commented Sep 30, 2024

Awesome stuff, Pat! Just checking out the branch now.

  1. I'm open to new UI/UX additions, but my question is maintainability and compatibility. Can shadcn eventually play nice with our setup? Would it be issues downstream if we have multiple UI libraries? Perhaps the actual look might become an issue. I personally favor sticking with one system as far as possible, and we mainly use Nextra's UI stuff with some minor additions from MUI and other things, but shadcn might be a nice addition.
  2. The link should change cursor to a hand like the other links in the vicinity.
  3. There is a slight pop when the modal opens where the entire page shifts due to the scrollbar disappearing.
  4. Not sure about adding an extra icon library when we already have MUI icons and it includes github icon.
  5. Next wasn't updated because Nextra runs on the older version (pages router). It might still be compatible but I just hadn't checked and the security features aren't are problem here.

Will try it some more later on and let you know if there's anything else, but this is a solid start!

@PatrickLarocque
Copy link
Author

PatrickLarocque commented Sep 30, 2024

Thanks for the feedback! I will be able to address most of your comments after my day job. Will get back to you shortly with some fixes.

@@ -39,4 +39,8 @@ next-env.d.ts
# Ignore generated credentials from google-github-actions/auth
gha-creds-*.json

# IDE
.idea
.vscode
Copy link
Member

Choose a reason for hiding this comment

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

What's the rationale for gitignoring .vscode?

Copy link
Author

@PatrickLarocque PatrickLarocque Sep 30, 2024

Choose a reason for hiding this comment

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

Some IDEs will autogenerate a .${ide} file with workspace specific preferences, like code style configuration, debug configuration, vcs settings, plugin configuration, and other cached behavior to apply to the workspace. These are specific to the developer's IDE, workspace and preferences and can vary based on the dev's setup.

I see that this repo has a .vscode file however, normally I find it best practice to exclude these, and rely on things like a conventional prettier.config to manage the behavior of linters or plugins, but I can remove the .vscode from the .gitinore file you would like to enforce the configuration outlined in yours. Although if someone changes them, or has some quirky vscode settings that get pushed, you will have to that at that point.

Up to you! Happy to remove this if you prefer!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I can see that, but I figured the dev would have put their preferred configs on a global level rather than workspace.

You can gitignore .ide and other things, but perhaps leave .vscode out of it. I think it's handy to have the extensions.json and settings.json as they serve as extension recommendations (which the dev can decline should they like) and some pretty essential markdown, mdx, plaintext configs (because otherwise the mdx files become impossible to work with given our literary purposes with them). I think for vscode the dev can overrride workspace settings with their own user settings? If it turns out to be an issue down the line, we can gitignore it.

@Firgrep
Copy link
Member

Firgrep commented Oct 1, 2024

Something you might consider is the MUI modal. Seems to handle the popping issue and comes with a nice transition effect.
https://mui.com/material-ui/react-modal/#transitions

@JMielbrecht
Copy link
Contributor

JMielbrecht commented Oct 1, 2024

@Firgrep Speaking from experience, using MUI can be a deal with the devil. MUI components offer great out-of-the-box functionality at the expense of customizability. They can be very difficult to customize depending on the component. If we wanted to change the look/feel of the app down the road, MUI could bite us in the behind.

From what I know of shad/cn, on the other hand, their components seem very customizable and transparent to developers. Of course, I've never used that library before, so @PatrickLarocque would be a better person to ask about that.

It might not be an issue in this case, however. Just a cautionary tale I wanted to bring up because I've been burned by MUI before.

@Firgrep
Copy link
Member

Firgrep commented Oct 1, 2024

Points taken and appreciated @JMielbrecht . While I agree that MUI components tend to be very finished, I've never had an issue with customizability (and we've been using it daily for a frontend at work for over a year). The sx prop virtually allows one to change any CSS. Of course, MUI comes with a certain look, but that is something we needn't follow, and we needn't use it always either. I just figured they have a very polished modal functionality we could use and inside of which we put in whatever.

I'm hesitant about shadcn because of (1) can the problem be adequately solved with existing tools? And (2) I'm unsure about the API and maintainability of external code that's copy-pasted directly rather than imported as a package. What if the components have a bug? Do we re-copy-paste individual bits, rather than just update a version of a package? But I'm open to hear more about this.

@Firgrep
Copy link
Member

Firgrep commented Nov 15, 2024

Closed due to going stale/no activity.

@Firgrep Firgrep closed this Nov 15, 2024
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.

Create a modal for "edit this page on Github"
3 participants