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

Bring in new menu, changing build step #14

Merged
merged 6 commits into from
Dec 14, 2017
Merged

Bring in new menu, changing build step #14

merged 6 commits into from
Dec 14, 2017

Conversation

njbotkin
Copy link
Owner

@njbotkin njbotkin commented Dec 9, 2017

I mostly want to know what you think about the rollup changes. I should have made a PR with just them separately. Bah.

@@ -8,6 +8,7 @@ client/data

public/*-bundle.js*
public/style.css*
public/components.css*
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe there should just be a public/*.css rule?

<div class="anythingelsebutton" on:click="set({visible: false})"></div>
{{/if}}

<div class="slide-out {{visible}}Visible">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd strongly recommend using data attributes instead of dynamic classes to represent state:

<div data-visible="{{visible}}" class="slide-out">

Then you can use the selectors [data-visible=true] and [data-visible=false].

rollup.config.js Outdated

preprocess: {
style({ content }) {
return new Promise(fulfill => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks legit to me. One possible change I see: since process() returns a thenable, you can resolve it directly with Promise.resolve instead of constructing your own Promise.

The main advantage there is that all errors/responses are represented in the new Promise, whereas right now this promise is only getting resolved if execution makes it all the way down to line 40 without any errors happening.

style({ content }) {
	return Promise.resolve(postcss([
		require(`precss`)({
			import: {
				path: [ `client/global-css` ],
				prefix: ``,
			},
		}),
		require(`autoprefixer`),
	])
		.process(content)
		.then(result => {
			return { code: result }
		}))
},

@njbotkin njbotkin merged commit d75de9f into master Dec 14, 2017
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.

2 participants