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

fix: Adjust gist link address bar #94

Merged
merged 27 commits into from
Sep 25, 2018
Merged

fix: Adjust gist link address bar #94

merged 27 commits into from
Sep 25, 2018

Conversation

BeniCheni
Copy link
Contributor

@BeniCheni BeniCheni commented Sep 5, 2018

Fixes: #85

Use a single <input> for the gist link address bar:

  • use Input with type=url
  • remove placeholder to leverage on value for gist link
  • update left padding style to look "okay"
  • update related test snapshot.

(P.S.: These changes are based on my best "guesses". Happy to update according to review & advice. 🙏🏻)

Test

test

Test w. token population

test-w-token

@BeniCheni BeniCheni changed the title fix: Adjust gist link address bar [WIP] fix: Adjust gist link address bar Sep 5, 2018
@BeniCheni BeniCheni changed the title [WIP] fix: Adjust gist link address bar fix: Adjust gist link address bar Sep 6, 2018
@felixrieseberg
Copy link
Member

Hey! Solid start, but there are a few issues here:

  1. Typing results in the url being repeated over and over again
  2. Would be nice to style the left side of the input box, too
  3. The existing box has some nice url coercion that might break.

This really is a solid start though!

@BeniCheni
Copy link
Contributor Author

Thank you, @felixrieseberg! I've updated accordingly. Please review again. 🙏
(Sorry about the delay. Been off the grid for other work stuff 📡. Now back to contribute to Electron Fiddle more "encore" 😸 )

  1. Typing results in the url being repeated over and over again

Tested the original bug I introduced to understand more. Reverted to use the original placeholder attribute & its value.

  1. Would be nice to style the left side of the input box, too

Inspected other input field's style more, & updated the style for the gist URL field's style.

  1. The existing box has some nice url coercion that might break.

Read the regex in the gist.ts logic here, & used the pattern attribute to validate the pattern of the nice url coercion logic. Please definitely advise if my assumption is incorrect.

@felixrieseberg
Copy link
Member

Hey @BeniCheni, good stuff! I found a few more small issues and decided to just fix them on your branch. I'm now just waiting to see if I broke any tests - if I didn't, I'll merge this later today!

@felixrieseberg felixrieseberg merged commit 44df77b into electron:master Sep 25, 2018
@BeniCheni BeniCheni deleted the gist-link-input branch October 6, 2018 15:17
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.

4 participants