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

feat: Update 'table view', 'alumni' and SLA status badges #595

Merged
merged 29 commits into from
Aug 20, 2020
Merged

feat: Update 'table view', 'alumni' and SLA status badges #595

merged 29 commits into from
Aug 20, 2020

Conversation

allisonsuarez
Copy link
Contributor

@allisonsuarez allisonsuarez commented Aug 18, 2020 •
edited
Loading

Summary of Changes

The "Table View" badge on tables and "Alumni" badge on users are not real badges since they are part of the structured metadata. Since you can't click on these and filter. We should update the design to not look like a badge. Similarly the SLA Hit/Missed status should be updated to no longer use badges.

Tests

Removed tests that specifically required the rendering of a Flag component on these pages and added ones to test the new design.

Documentation

N/A

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes, including screenshots of any UI changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public python functions and the classes in the PR contain docstrings that explain what it does
  • PR passes all tests documented in the developer guide

@allisonsuarez allisonsuarez requested review from Golodhros, ttannis and dikshathakur3119 and removed request for Golodhros August 18, 2020 23:39
@ttannis ttannis mentioned this pull request Aug 18, 2020
Closed
5 tasks
Golodhros
Golodhros reviewed Aug 19, 2020
View reviewed changes
Copy link
Member

@Golodhros Golodhros 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

@allisonsuarez allisonsuarez force-pushed the asm-replace-flag branch 2 times, most recently from fd4292a to ad41fcb Compare August 19, 2020 20:21
ttannis
ttannis reviewed Aug 19, 2020
View reviewed changes
Copy link
Contributor

@ttannis ttannis 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 but some more comments on the dashboard UI, the task is to change how the information is displayed, but not to change the text...which is why the text is a property so that the parent can control what text is shown for the positive and negative cases.

Additionally, do you have a screen shot to post? On the slack I remember seeing some slight alignment issues and so the screenshot will help to see if that's still the case. Thanks!

amundsen_application/static/css/_icons.scss Show resolved Hide resolved
amundsen_application/static/js/components/DashboardPage/constants.ts Show resolved Hide resolved
amundsen_application/static/js/components/DashboardPage/index.tsx Show resolved Hide resolved
@allisonsuarez
removed badges section for users cause we dont need that moving forward
7350d82 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
removed ALumni badge from user profile and added it to header-bullets…
dd13c8d 
… instead

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
removed Alumni badge checks for user profiles
dcfe0c5 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
not working for some reason
2c07e1e 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
lint and missed test update
7c6267c 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
fixed conflict
576fdab 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
fixed alignment issue
3abea6d 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
fixed table view
5e1c02a 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
added variables for style
c9e1668 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
made component more genric, still need to rename and fix icon issue
e157509 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
updated text to use typography defined
6c2c16e 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
variable
e2b7c1f 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
hit icon broken
1823001 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
fixed hit icon a bit
5ba4f1d 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
fixed tets
f4640e8 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
moved setup into it clause
d0b6e1b 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
added resource badges back in
1121cad 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
lint + added back tests
c9e739f 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
fixes
4d6b782 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
lint
03d0f5b 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
fixed icon for hit
a9eb676 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
bettered updated file
1f9e1cf 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Golodhros
Golodhros previously approved these changes Aug 19, 2020
View reviewed changes
Copy link
Member

@Golodhros Golodhros left a comment

Choose a reason for hiding this comment

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

LGTM

@allisonsuarez
removed text prop
30f5cd1 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
fixed text issue
691d2b6 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
small fix
5d4a7d1 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez
cleanup
6adc003 
Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
@allisonsuarez allisonsuarez merged commit d2f222e into master Aug 20, 2020
@allisonsuarez allisonsuarez deleted the asm-replace-flag branch August 20, 2020 00:08
jerryzhu2007 pushed a commit to kylg/amundsenfrontendlibrary that referenced this pull request Aug 20, 2020
Merge commit 'd2f222ea5cb648fb4a9d9bd2e242a3b36281098d'
3df4947 
* commit 'd2f222ea5cb648fb4a9d9bd2e242a3b36281098d': (63 commits)
  feat: Update 'table view', 'alumni' and SLA status badges (amundsen-io#595)
  build: Adding betterer to workflow (amundsen-io#598)
  build(deps): bump flask-restful from 0.3.7 to 0.3.8 (amundsen-io#553)
  chore: Removes 84 unused variables (amundsen-io#600)
  chore: remove travis badge and update doc link (amundsen-io#599)
  Fixes error on TableDetail on Dev (amundsen-io#597)
  chore: Bump version for pypi release. (amundsen-io#596)
  feat: Announcements in Homepage (amundsen-io#591)
  build(deps-dev): bump @babel/plugin-proposal-logical-assignment-operators (amundsen-io#574)
  build(deps): [security] bump lodash in /amundsen_application/static (amundsen-io#587)
  build(deps-dev): bump @babel/plugin-proposal-export-namespace-from (amundsen-io#588)
  build(deps-dev): bump lint-staged in /amundsen_application/static (amundsen-io#590)
  chore: fix docker push action (amundsen-io#593)
  feat: add github actions for FE (amundsen-io#592)
  chore: Re-useable OwnerEditor (amundsen-io#548)
  feat: Show more tags in browse and home  (amundsen-io#532)
  Organizes, themes and adds typography stories (amundsen-io#578)
  docs: describe storybook (amundsen-io#577)
  Fixes TypeScript issues with sagas and saga tests (amundsen-io#573)
  build(deps-dev): bump @babel/core in /amundsen_application/static (amundsen-io#558)
  ...

# Conflicts:
#	amundsen_application/api/utils/metadata_utils.py
#	amundsen_application/oidc_config.py
#	amundsen_application/static/js/components/TableDetail/ColumnList/index.tsx
#	amundsen_application/static/js/components/TableDetail/ColumnListItem/index.tsx
#	amundsen_application/static/js/components/TableDetail/constants.ts
#	amundsen_application/static/js/components/TableDetail/index.tsx
#	amundsen_application/static/js/config/index.spec.ts
#	amundsen_application/static/js/ducks/tableMetadata/reducer.ts
#	amundsen_application/static/js/fixtures/globalState.ts
#	amundsen_application/static/js/fixtures/metadata/table.ts
#	amundsen_application/static/js/interfaces/TableMetadata.ts
#	docs/application_config.md
#	requirements.txt
#	setup.py
ttannis pushed a commit that referenced this pull request Aug 21, 2020
@allisonsuarez @ttannis
feat: Update 'table view', 'alumni' and SLA status badges (#595)
5777ce8 
* removed badges section for users cause we dont need that moving forward

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* removed ALumni badge from user profile and added it to header-bullets instead

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* removed Alumni badge checks for user profiles

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* not working for some reason

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint and missed test update

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* fixed conflict

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* updated DashboardPage tests

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* added icons

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* fixed alignment issue

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* fixed table view

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* added variables for style

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* made component more genric, still need to rename and fix icon issue

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* updated text to use typography defined

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* variable

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* hit icon broken

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* fixed hit icon a bit

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* fixed tets

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* moved setup into it clause

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* added resource badges back in

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint + added back tests

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* fixes

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* lint

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* fixed icon for hit

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* bettered updated file

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* removed text prop

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* fixed text issue

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* small fix

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>

* cleanup

Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Signed-off-by: Tamika Tannis <ttannis@lyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ttannis ttannis ttannis approved these changes

@dikshathakur3119 dikshathakur3119 Awaiting requested review from dikshathakur3119 dikshathakur3119 is a code owner

@Golodhros Golodhros Awaiting requested review from Golodhros Golodhros is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@allisonsuarez @Golodhros @ttannis