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

Feature: Add tabs and footer #137

Merged

Conversation

stephenoldham
Copy link
Contributor

Adds new header/footer layout and content. Also adds new tabs functionality, not currently enabled when using the panel in production.

Removed edge files from prettier format run because using the html parser breaks components and includes based on incorrect formatting of tags.

@stephenoldham stephenoldham changed the title Feature/add tabs and footer Feature: Add tabs and footer Dec 21, 2020
Copy link
Collaborator

@HugoDF HugoDF left a comment

Choose a reason for hiding this comment

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

Nice one, just a couple of comments

We kind of need to make a decision around whether we want to lean on Edge.js templating a bit more or not (eg. for hiding work in progress features in production builds), will affect #136 as well

packages/shell-chrome/views/_partials/footer.edge Outdated Show resolved Hide resolved
packages/shell-chrome/views/_partials/footer.edge Outdated Show resolved Hide resolved
:href="isLatest ? '#' : 'https://github.com/alpinejs/alpine/releases'"
x-text="detected"
></a>
<span x-text="loadingText"></span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we move this loading state to display inside the panel? I think it's fine here, but maybe there's a more obvious spot for it.

Copy link
Contributor Author

@stephenoldham stephenoldham Dec 23, 2020

Choose a reason for hiding this comment

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

Thought I'd have a play with some animation...too much?

Updated Version
If we are into this...this version is a bit better with a mono font below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it...

too much?

Probably :) I'm personally okay with it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean...it is a little too much really 😆

Just the text with a blinking cursor in the components panel might be the one.

Copy link
Contributor

@KevinBatdorf KevinBatdorf Dec 23, 2020

Choose a reason for hiding this comment

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

I'm probably not the best person to judge. Maybe let Hugo or Ryan chime in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Haha @stephenoldham let's go for blinking text

I guess we can always add a nicer animation down the line

Copy link
Contributor

@KevinBatdorf KevinBatdorf left a comment

Choose a reason for hiding this comment

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

I suggest we go with the more generic devOnly so it can be used in other places as well.

lib/edge/render.js Outdated Show resolved Hide resolved
packages/shell-chrome/views/_partials/header.edge Outdated Show resolved Hide resolved
packages/shell-chrome/views/_partials/header.edge Outdated Show resolved Hide resolved
packages/shell-chrome/views/panel.edge Outdated Show resolved Hide resolved
@HugoDF
Copy link
Collaborator

HugoDF commented Jan 5, 2021

@stephenoldham @KevinBatdorf shall we merge this? We can improve the loading state later, this would mean @ryangjchandler's events tab PR can get moved on and so can #126

@KevinBatdorf
Copy link
Contributor

Were we just waiting on animations? Can merge now and do that separately anyway.

@HugoDF
Copy link
Collaborator

HugoDF commented Jan 5, 2021

Were we just waiting on animations? Can merge now and do that separately anyway.

I believe so, I've taken this for a spin again and it all looks good (+ tests are passing) so should be safe to merge.

@HugoDF HugoDF merged commit 4728131 into alpine-collective:master Jan 5, 2021
@stephenoldham
Copy link
Contributor Author

Sorry guys, in the throws of break/back to work. Yeah loading animation can wait 👍🏼

@HugoDF HugoDF added this to the next [unreleased] milestone Jan 12, 2021
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