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

Fixed the delete last token from API token table list #3591

Merged
merged 3 commits into from
May 14, 2020

Conversation

vinay033
Copy link
Collaborator

@vinay033 vinay033 commented May 5, 2020

Signed-off-by: vinay033 vsharma@chef.io

πŸ”© Description: What code changed, and why?

user cannot delete the last token in the table. The last option in the control menu is cut off. This is not a problem on any of the other auth pages since they all only have one control menu option.

I have added CSS changes to fix this issue.

⛓️ Related Resources

fixes #2798

πŸ‘ Definition of Done

I have added mat-select-panel CSS to fix for last element of table list.

πŸ‘Ÿ How to Build and Test the Change

STEP 1
inside the hab studio

[default:/src:0]# build components/automate-ui-devproxy/
[default:/src:0]# start_automate_ui_background
[default:/src:0]# start_all_services

STEP 2
open new window
go to automate UI path

$ cd components/automate-ui
and run the command 

npm run serve:hab

βœ… Checklist

πŸ“· Screenshots

  1. old behavior
    API-tokenold
  2. after fix
    API-teken

@vinay033 vinay033 self-assigned this May 5, 2020
@susanev susanev requested a review from a team May 5, 2020 10:47
@susanev
Copy link
Contributor

susanev commented May 5, 2020

@vinay033 only looking at the screenshots, could you push the menu up a bit more, so you can still see the 3-dot button? like when it displays below, but instead above.

@vinay033
Copy link
Collaborator Author

vinay033 commented May 5, 2020

@vinay033 only looking at the screenshots, could you push the menu up a bit more, so you can still see the 3-dot button? like when it displays below, but instead above.

@susanev if we add changes to push menu up then it will be applying for both cases(when the menu opens down and when the menu opens in up for the last element) and the default position is changed for down.

  1. up
    up
  2. down
    down

@susanev
Copy link
Contributor

susanev commented May 5, 2020

all right, imma post in the ui team and see if any of them have ideas. ideally, it would reflect symmetrically over the 3-dot button.

reflect

@vinay033
Copy link
Collaborator Author

vinay033 commented May 5, 2020

all right, imma post in the ui team and see if any of them have ideas. ideally, it would reflect symmetrically over the 3-dot button.

reflect

we can alternate show like this.
newdown
newup

@SEAjamieD
Copy link
Contributor

@susanev - i'm having a quick look at this - is the idea that when the control menu goes up, the list should be reversed as well?

@susanev
Copy link
Contributor

susanev commented May 5, 2020

@SEAjamieD no no, that doesnt need to change, only its position on the screen. i dont want it hiding the 3-dot button.

@SEAjamieD
Copy link
Contributor

SEAjamieD commented May 6, 2020

@susanev This is gonna take some wrestling with Material to figure out. Material naturally changes direction by default when it gets close to the bottom of the screen, but because I overwrote a bunch of the styles to fit our designs, it can't fire the way it's designed.

After desktop is done, i'll keep looking. @tarablack01 maybe has some ideas?

@susanev
Copy link
Contributor

susanev commented May 6, 2020

not a ton of urgency, other than we may want to close this pr and try again later.

@tarablack01
Copy link
Contributor

tarablack01 commented May 6, 2020

@vinay033 What you can do is change the popover position if the item is last or lower in the list. So that you can have a popover position of above and below. Typically material does this automatically but as @SEAjamieD is may have been overwritten, if so we could also look into reestablishing the functionality. Ping me if you need assistance. πŸ™‚

@vinay033 vinay033 force-pushed the Vinay/mat-select-penal-fix branch from 8a2dade to 4ee649d Compare May 8, 2020 07:05
@SEAjamieD
Copy link
Contributor

SEAjamieD commented May 8, 2020

This is great @vinay033! This restores the initial functionality of the button so that it works the way material intended. Once UX has a say here, can you move this into styles.scss and replace the styling that is currently under chef-control-menu? This should be used globally.

This exposed a second problem, which I never noticed before that @susanev will need to answer. So, it appears the reason why Material, by default, has the dropdown display over the button, is because the button has no functionality when the menu is open.

To see what I mean, click on the control button to open the menu. Then, while the menu is open, click on the button again to close it. Nothing happens. It appears default functionality is "click outside to close". Update suggestions?

This also may be happening because we're sort of hacking it to work the way we want. This is definitely one of the components I'd like us to rebuild ourselves. Its not hugely complicated and its used a lot.

Copy link
Contributor

@SEAjamieD SEAjamieD 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 this is great @vinay033 - I left you a comment, requesting that you move these styles to the styles.scss file so that they are global!

There's another comment in there for UX so i'll wait to hear from them until approving. Thank you!

@vinay033
Copy link
Collaborator Author

I think this is great @vinay033 - I left you a comment, requesting that you move these styles to the styles.scss file so that they are global!

There's another comment in there for UX so i'll wait to hear from them until approving. Thank you!

These changes do not work globally because I have tested after putting this to style.scss file,
it is working fine with the token list but other places it looks like below
del1
del2

@susanev susanev requested review from SEAjamieD and a team May 11, 2020 16:43
@susanev susanev added the bug πŸ› Something isn't working label May 11, 2020
Copy link
Contributor

@susanev susanev left a comment

Choose a reason for hiding this comment

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

thank you for fixing this!!

@susanev
Copy link
Contributor

susanev commented May 11, 2020

just noticed that we seem to have lost toggle functionality on that 3-dot button. anyone know whats up with that?

@susanev susanev self-requested a review May 11, 2020 16:56
@susanev
Copy link
Contributor

susanev commented May 11, 2020

oh lol, apologies for not scrolling up to see jamies comments. id like to either (1) get toggle working (2) display over the button as jamie described. either is okay with me.

@vinay033 vinay033 force-pushed the Vinay/mat-select-penal-fix branch 2 times, most recently from 262f1c2 to e35fa16 Compare May 13, 2020 15:30
@susanev
Copy link
Contributor

susanev commented May 13, 2020

@vinay033 im still not seeing the toggle fixed in this?

@vinay033
Copy link
Collaborator Author

@vinay033 im still not seeing the toggle fixed in this?

@susanev toggle is working, what issue you are facing with a toggle so I can explain to you.

@susanev
Copy link
Contributor

susanev commented May 13, 2020

@vinay033 oh i dont mean the toggle item in the dropdown. i mean the 3-dot button. previously if you pressed the 3-dot menu it would open, but if you pressed it again it would close. right now, you can press it to open but pressing it to close does not close it. happy to zoom to demo anytime.

@vinay033
Copy link
Collaborator Author

@vinay033 oh i dont mean the toggle item in the dropdown. i mean the 3-dot button. previously if you pressed the 3-dot menu it would open, but if you pressed it again it would close. right now, you can press it to open but pressing it to close does not close it. happy to zoom to demo anytime.

Ok, I will check it again and get back to you. :)

Signed-off-by: vinay033 <vsharma@chef.io>
Signed-off-by: vinay033 <vsharma@chef.io>
Signed-off-by: vinay033 <vsharma@chef.io>
@vinay033 vinay033 force-pushed the Vinay/mat-select-penal-fix branch from e35fa16 to a3740e6 Compare May 14, 2020 12:09
@vinay033
Copy link
Collaborator Author

@vinay033 oh i dont mean the toggle item in the dropdown. i mean the 3-dot button. previously if you pressed the 3-dot menu it would open, but if you pressed it again it would close. right now, you can press it to open but pressing it to close does not close it. happy to zoom to demo anytime.

@susanev I have fixed the toggle issue, please check it.

Copy link
Contributor

@tarablack01 tarablack01 left a comment

Choose a reason for hiding this comment

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

Toggles 😊

@tarablack01 tarablack01 dismissed SEAjamieD’s stale review May 14, 2020 14:48

This shouldn't be in global css file

@susanev susanev merged commit 1851de1 into master May 14, 2020
@chef-expeditor chef-expeditor bot deleted the Vinay/mat-select-penal-fix branch May 14, 2020 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automate-ui bug πŸ› Something isn't working ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't delete last token in the table
4 participants