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.

Sign up for GitHub

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

Jump to bottom

Create ActionCaption component #1739

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Create ActionCaption component #1739

merged 2 commits into from
Apr 16, 2021

Conversation

marcoambrosini
Copy link
Contributor

@marcoambrosini marcoambrosini commented Mar 3, 2021 •
edited
Loading

Upstreaming talk's actioncaption, as per the original @jenniferpiperek design

Screenshot from 2021-03-03 15-52-30

Signed-off-by: Marco Ambrosini marcoambrosini@pm.me

@marcoambrosini marcoambrosini self-assigned this Mar 3, 2021
@marcoambrosini marcoambrosini added 3. to review Waiting for reviews component Component discussion and/or suggestion feature: actions Related to the actions components labels Mar 3, 2021
nickvergessen
nickvergessen reviewed Mar 3, 2021
View reviewed changes
Copy link
Contributor

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

I think the color is too much inside an action component. Its okay in the navigation component but not in the action popover menu

src/components/ActionCaption/ActionCaption.vue Outdated Show resolved Hide resolved
@marcoambrosini marcoambrosini force-pushed the action-caption branch 2 times, most recently from a059b64 to 820aedc Compare March 3, 2021 16:31
@marcoambrosini
Copy link
Contributor Author

I think the color is too much inside an action component. Its okay in the navigation component but not in the action popover menu

Imo it creates a nice separation between actions and caption, but I don't have a strong opinion here. @jancborchardt your call

@PVince81 PVince81 mentioned this pull request Mar 4, 2021
Merged
@marcoambrosini marcoambrosini mentioned this pull request Mar 4, 2021
Closed
11 tasks
@PVince81 PVince81 mentioned this pull request Mar 8, 2021
Closed
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 16, 2021
skjnldsv
skjnldsv requested changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Waiting fro @jancborchardt's return

@jancborchardt
Copy link
Contributor

I think the color is too much inside an action component. Its okay in the navigation component but not in the action popover menu

Imo it creates a nice separation between actions and caption, but I don't have a strong opinion here. @jancborchardt your call

I would agree the color and boldness is too much for just a caption – since the options themselves are the important thing. I would actually set the caption in var(--color-text-maxcontrast) (and possibly not bold) and use the color (and maybe the boldness, but probably not) for the selected option instead.

@marcoambrosini
Copy link
Contributor Author

Allright, ready to review again

Screenshot from 2021-04-16 09-19-54

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 16, 2021
skjnldsv
skjnldsv approved these changes Apr 16, 2021
View reviewed changes
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Let's use variables 🚀

@marcoambrosini
Create ActionCaption component
5f55065 
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
PVince81
PVince81 approved these changes Apr 16, 2021
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Contributor

ohhh, a conflict!

@marcoambrosini marcoambrosini merged commit 5653d19 into master Apr 16, 2021
@marcoambrosini marcoambrosini deleted the action-caption branch April 16, 2021 14:17
@skjnldsv skjnldsv mentioned this pull request Jun 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees

@marcoambrosini marcoambrosini

Labels
3. to review Waiting for reviews component Component discussion and/or suggestion feature: actions Related to the actions components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marcoambrosini @jancborchardt @PVince81 @nickvergessen @skjnldsv