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

update tabs on tiles #1064

Merged
merged 60 commits into from
Oct 5, 2021
Merged

update tabs on tiles #1064

merged 60 commits into from
Oct 5, 2021

Conversation

stackoverfloweth
Copy link
Contributor

@stackoverfloweth stackoverfloweth commented Sep 28, 2021

PR Checklist:

  • add a short description of what's changed to the top of the CHANGELOG.md
  • add/update tests (or don't, for reasons explained below)

Describe this PR

src/pages/Flow/UpcomingRuns-Tile.vue Outdated Show resolved Hide resolved
src/pages/Flow/UpcomingRuns-Tile.vue Outdated Show resolved Hide resolved
Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

One tiny nitpick - there's a border above 'options' on the late run tab that I don't think we want/need. it's especially obvious in dark mode.

This PR:

Screen Shot 2021-09-30 at 5 11 23 PM

Screen Shot 2021-09-30 at 5 11 36 PM

Current UI:

Screen Shot 2021-09-30 at 5 12 19 PM

Screen Shot 2021-09-30 at 5 12 32 PM

Otherwise looks good to me. Once those small changes are made, I'd welcome a final review from @znicholasbrown, @whimsicallyson and/or @pleek91

@stackoverfloweth stackoverfloweth requested a review from zhen0 October 1, 2021 15:37
@zhen0
Copy link
Member

zhen0 commented Oct 4, 2021

Looks good to me - can we get another approval before merging to staging? @pleek91 @whimsicallyson (Once that review is done, if you're happy you can remove the do not merge label)

@zhen0 zhen0 added the do not merge Do Not Merge label Oct 4, 2021
Copy link

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

Looks good @stackoverfloweth

@whimsicallyson
Copy link
Contributor

my only question/comment is how it works if there are more tab options than the width of the card. does it overflow properly in the horizontal, or do we wend up with the same issue that we had with the old implementation?

@stackoverfloweth
Copy link
Contributor Author

my only question/comment is how it works if there are more tab options than the width of the card. does it overflow properly in the horizontal, or do we wend up with the same issue that we had with the old implementation?

@whimsicallyson Great point, yes it does overflow properly! That's built into v-tabs https://vuetifyjs.com/en/components/tabs/#dynamic-tabs.
You can see an example of this on the tile reference in the original bug (can have up to 4 tabs)

@zhen0 zhen0 removed the do not merge Do Not Merge label Oct 4, 2021
@zhen0 zhen0 merged commit b471f2d into master Oct 5, 2021
@zhen0 zhen0 deleted the issues/1056 branch October 5, 2021 17:48
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.

BUG: Squished Flow Run detail options if there are more than two
5 participants