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

Replace primer.css with webext-base-css #895

Merged
merged 11 commits into from
Apr 19, 2020
Merged

Replace primer.css with webext-base-css #895

merged 11 commits into from
Apr 19, 2020

Conversation

fregante
Copy link
Collaborator

@fregante fregante commented Apr 17, 2020

Fixes #893 using webext-base-css and a little extra CSS.

  • Restore error via inputField.setCustomValidity(error)

⬆️ I don't know how to tackle that, feel free to edit this PR.

ff-white

ff-black

ch-white

ch-black

@fregante
Copy link
Collaborator Author

The remaining lint error is deprecated:

##[error] 13:7 error Form label must have ALL of the following types of associated control: nesting, id jsx-a11y/label-has-for

@stefanbuck
Copy link
Member

Thanks for the Pull Request @fregante. According to https://stackoverflow.com/a/21458090/2121324 .reportValidity() does the trick to force a revalidation. It's already quite late, but I can have a look myself tomorrow.

@@ -2,6 +2,7 @@
<head>
<meta charset="UTF-8" />
<title></title>
<link rel="stylesheet" href="chrome://global/skin/in-content/common.css">
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if this is supposed to work in MS Edge as well? The readme of webext-base-css mentions just Firefox. I'm asking because it looks like they added their own protocol edge://

Copy link
Collaborator Author

@fregante fregante Apr 17, 2020

Choose a reason for hiding this comment

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

Indeed, this only loads in Firefox. Firefox effectively doesn't support options.chrome_style so thankfully this stylesheet fills in.

I don't know how it looks in Edge, but I assume it just matches Chrome.

@xt0rted
Copy link
Member

xt0rted commented Apr 17, 2020

This is what it looks like in Edge 83.0.478.10 (Official build) dev (64-bit) in Win10

image

This is the current published version in 81.0.416.53 (Official build) (64-bit) in Win10

image

@fregante
Copy link
Collaborator Author

fregante commented Apr 17, 2020

.reportValidity() does the trick to force a revalidation.

I think that's the wrong one, you'll need .setCustomValidity because .reportValidity only reports the error... but the browser doesn't know the error.

We used .reportValidity() in RG: refined-github/refined-github@cb37f6d#diff-4bfcd665876550ba5b46897389349245L13

I just don't know how to integrate that with Preact 😅

@fregante
Copy link
Collaborator Author

fregante commented Apr 17, 2020

This is what it looks like in Edge 83.0.478.10 (Official build) dev (64-bit) in Win10

Not too shabby! Only issues:

  • drop width on html
  • the dark color doesn't match

The second point should be taken care by webext-base-css, but I don't think there's currently a way to target Edge via CSS like I do for Firefox.

@xt0rted
Copy link
Member

xt0rted commented Apr 17, 2020

This is it after the last update.

image

@xt0rted
Copy link
Member

xt0rted commented Apr 17, 2020

I also noticed the yarn.lock has some changes that weren't committed.

@fregante

This comment has been minimized.

@xt0rted
Copy link
Member

xt0rted commented Apr 18, 2020

I ran yarn install in the root of the project. I'm new to yarn but that seems like all I needed to do.

@stefanbuck
Copy link
Member

I think that's the wrong one, you'll need .setCustomValidity because .reportValidity only reports the error... but the browser doesn't know the error.

From my understanding you need both if your are not using proper form submission like we do. I'm on it now

this.tokenInputEl.setCustomValidity(
response.message || 'Something went wrong',
);
this.tokenInputEl.reportValidity();
Copy link
Member

Choose a reason for hiding this comment

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

This solution is inspirited by https://github.com/developit/preact-mdl/blob/master/src/index.js#L873-L878 so it shouldn't be completely wrong 😉

@stefanbuck
Copy link
Member

@fregante any thought on styling the success message different when using dark mode or not?

image

@fregante
Copy link
Collaborator Author

fregante commented Apr 19, 2020

Instead of adding a new element above the field, what do you think about replacing the field description with:

✅ Token successfully saved.

  • It doesn’t move the field down
  • The green color is just given by the emoji, no need for further CSS

});
this.tokenInputEl.setCustomValidity(
response.message || 'Something went wrong',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should the default message be more specific? Like “The token could not be validated”

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Also the API response is not very descriptive Bad credentials in my opinion. Will change it later

@stefanbuck
Copy link
Member

The success message reserves some space in the layout to avoid form jumping when submitting. I tried out the other approach suggested by @fregante, but it felt wrong to hide the input field without showing a back button and at this point I decided for the simple approach

@fregante
Copy link
Collaborator Author

fregante commented Apr 19, 2020

hide the input field without showing a back button

Hmm I didn’t suggest that, but replacing only the 1-line description below the field.

That looks good though

@stefanbuck
Copy link
Member

I should read comments more carefully 😊

Thanks for this very nice collaboration

@stefanbuck stefanbuck merged commit 0753b9e into OctoLinker:master Apr 19, 2020
@fregante fregante deleted the light-options branch April 19, 2020 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add dark color scheme support in the options
3 participants