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

Respond to duke feedback #17

Merged
merged 15 commits into from
Aug 14, 2024
Merged

Respond to duke feedback #17

merged 15 commits into from
Aug 14, 2024

Conversation

C-Loftus
Copy link
Member

@C-Loftus C-Loftus commented Aug 7, 2024

Description

  • Gets rid of the fetched markdown in the footer. This is not the central source of truth, the docs website is, and it refers to Github info that isn't relevant.
  • Adds more detail to the CSV formatting reference and has it open by default.
    • For more detail, I am still linking to the docs. I think it is better to link to the docs rather than include big blocks of text that duplicate info we have on the docs site.
  • Added an additional details admonition block that calls out where to find specific useful info in the docs

Copy link

netlify bot commented Aug 7, 2024

Deploy Preview for register-geoconnex-us ready!

Name Link
🔨 Latest commit 152659b
🔍 Latest deploy log https://app.netlify.com/sites/register-geoconnex-us/deploys/66bc9e75058e1f0008590cab
😎 Deploy Preview https://deploy-preview-17--register-geoconnex-us.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@C-Loftus C-Loftus requested a review from ksonda August 7, 2024 19:39
@ksonda
Copy link
Member

ksonda commented Aug 8, 2024

Suggest something aboiut how what you put in the namespace will be created if it does not already exist, and your identifiers will just be added to the existing namespace if it already exists. And maybe slightly more explicit language that

  1. Identifiers are meant for individual monitoring locations
  2. Identifiers are meant to redirect to a unique, individual page for each monitroing location

@C-Loftus
Copy link
Member Author

C-Loftus commented Aug 8, 2024

Responded to the feedback.

  • Added addition info for the Identifiers. Wasn't sure where was best, but put it under the id description in the CSV info section.
  • Added an italic callout for the fact that the namespace will be either created or updated
  • Added an h2 element above the readme upload to specify it is either add or update

image

@webb-ben
Copy link
Member

Remove marked from package.json please

@webb-ben
Copy link
Member

webb-ben commented Aug 12, 2024

The page about the namespace creation should be a separate "step" from the uploading of the CSV. I suggest the implementation of stepper to better address the namespace specific requirements. Perhaps this is a first step where you either select the namespace from GitHub API populated list, or create a new one.

Second step is uploading and formatting the CSV file

@C-Loftus C-Loftus marked this pull request as draft August 12, 2024 20:27
@C-Loftus
Copy link
Member Author

@webb-ben when you get a moment would you mind letting me know what you think of the v-stepper change?

It is not done yet, still have to make the validation code work in each tab and improve the submission tab. (Wanted to check with you before I went in too deep or if you want anything else)

Thanks for recommending to use v-stepper. It looks nice and is a great way to split up the form semantically. (Also allows us to add extra background info without cramming the form)

Copy link
Member Author

@C-Loftus C-Loftus left a comment

Choose a reason for hiding this comment

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

Still more work to be done on this but switching off for a bit to work on some other stuff since I am getting a bit stuck. @webb-ben if you have a moment these were the two issues I was preferring to.

Otherwise I will take a look at thisagain later

src/components/MetadataGenerator.vue Outdated Show resolved Hide resolved
src/components/PageHeader.vue Outdated Show resolved Hide resolved
@C-Loftus
Copy link
Member Author

Since we do not have a logo that works with a dark background, should I just remove the dark mode toggle? I think dark mode is a nice quality of life improvement, but that being said, I can't find a way to make the logos look good on a dark background without adding a white border. Not sure what is best

@webb-ben
Copy link
Member

Since we do not have a logo that works with a dark background, should I just remove the dark mode toggle? I think dark mode is a nice quality of life improvement, but that being said, I can't find a way to make the logos look good on a dark background without adding a white border. Not sure what is best

I think we can implement it in a future sprint if desired. No reason to be hung up on it right now

@C-Loftus C-Loftus marked this pull request as ready for review August 14, 2024 12:01
@C-Loftus
Copy link
Member Author

Should be ready to review:

Things I added:

  • Stepper component breaks up the form into logical parts
  • Added some more verbiage/headers
  • Refactored code based to be more modular

Things I will add in a future PR:

  • Started dark mode but we don't have logos for a dark theme for not a good fit at the moment
  • Will refactor to use pinia for state management once the logic gets more complex

Things I wasn't sure about:

  • Unclear how much information we want to put on the last submission tab (too much potentially confuses them, but not having enough makes it unclear where the data was actually submitted to). I could link to the GitHub pull request once it is submitted but once again not sure if this is helpful or adds more confusion.

@webb-ben webb-ben self-requested a review August 14, 2024 13:23
@webb-ben webb-ben merged commit 70b38ec into main Aug 14, 2024
6 checks passed
@webb-ben webb-ben deleted the dukeRespond branch August 14, 2024 13:38
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