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

Add components to Vue Boilerplate #1

Closed
wants to merge 9 commits into from
Closed

Add components to Vue Boilerplate #1

wants to merge 9 commits into from

Conversation

sknep
Copy link

@sknep sknep commented Jun 30, 2020

[draft - not ready to merge]

My goals for this PR were to:

  • Demonstrate importing the new astro-static npm package and setting up a Vue Webpack project
  • Demonstrate a simple full-window layout using the global status bar and the tree using minimal flexbox and existing Astro css variables
  • Demonstrate defining both simple (boolean) and complex (object or array) properties in Vue, and binding or passing that data down to child components
  • Demonstrate how a click handler/method can be assigned to an Astro component
  • Demonstrate 2-way data binding or a props down/events up parent/child communication

I ran into some challenges which are described on the ticket in Jira: https://rocketcom.atlassian.net/browse/ASTRO-248?focusedCommentId=11997 and abridged here:

  • the Astro components are not actually reflecting prop changes in the DOM, which means there’s no way for vue to know when something in a child component has been selected/changed state/etc. I was not able to get the theme toggle component to reflect any changed state back to the parent. I can set the properties of any component on mount or any time I want, but I cannot seem to ‘get’ any data from the child components as they are today. Either we accept that the astro components are sort of one-way, or we will need to add events that notify the parent when a property has changed.
  • Using the global status bar, tree, and notification components together on the same page required some extra fiddly css jury-rigging with position, overflow, and z-index -- they don't play together by default. My recommendation here would be to simplify the internal CSS within the components so that they can work by default, rather than expect an implementer to have a deep knowledge of flexbox, overflow, and position hacks.

Additionally, I found that the fonts didn't load properly from the astro-static npm package because all the URLs are absolute. I strongly recommend (RocketCommunicationsInc/astro-components#3 (comment)) changing the urls in that package to relative within the css (cc @im-cr ) unless there's an implementation solution I'm missing entirely.

I'd very, very much like to know how Todd and Cortney are handling the asset imports and any other data binding for the React (https://rocketcom.atlassian.net/browse/ASTRO-249) and Angular (https://rocketcom.atlassian.net/browse/ASTRO-247) tickets.

This PR cannot be merged until the astro-static assets 'work' out of the box, as I've had to modify the font URLs in the src within node_modules to prototype this, and that won't do for a release.

@sknep sknep self-assigned this Jun 30, 2020
@sknep sknep requested review from dmcalester and im-cr June 30, 2020 15:22
@sknep sknep marked this pull request as ready for review June 30, 2020 15:37
@sknep sknep marked this pull request as draft June 30, 2020 15:37
"@astrouxds/rux-clock": "^4.0.0",
"@astrouxds/rux-accordion": "^4.0.0-alpha.3",
"@astrouxds/rux-button": "^4.0.2",
"@astrouxds/rux-clock": "^4.0.2",
"@astrouxds/rux-global-status-bar": "^4.0.0",
"@astrouxds/rux-icon": "^4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a reason only come of the packages were updated to their most recent versions? I would recommend upgrading button, icon, monitoring icon and clock to reflect the most recent changes.

Copy link
Author

Choose a reason for hiding this comment

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

  • Review each astro dependency and update accordingly

Copy link

@amalyah amalyah left a comment

Choose a reason for hiding this comment

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

Quick spellcheck review and questions.

</head>
<body>
<div id="app"></div>
<script src="/dist/build.js"></script>
<!-- built files will be auto injected -->
Copy link

Choose a reason for hiding this comment

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

Not sure if this is applicable here, but...

auto-injected

@@ -1,18 +1,24 @@
# Astro Vue Bootstrap
Copy link

Choose a reason for hiding this comment

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

Should this be Boilerplate instead?

"version": "1.0.0",
"author": "Rocket Communications",
"license": "MIT",
"name": "astro-vue",
Copy link

Choose a reason for hiding this comment

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

Is there a reason not to include a description, author, or license any more? Or to remove the term Boilerplate from he name?

margin-left: auto;
align-self: flex-end;
}
rux-accordion [slot='content'] {
Copy link

Choose a reason for hiding this comment

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

Is it okay that this line says content and the next one says contents (plural)?

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.

None yet

3 participants